impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options
Date Sat, 30 Sep 2017 00:20:04 GMT
Lars Volker has posted comments on this change. ( )

Change subject: IMPALA-5425: Add test for validating input when setting query options

Patch Set 10: Code-Review+1


I read through the code, but I still found it difficult to understand. Partially it seems
to be a more concise reimplementation of the server-side code. Ideally we'd rework our server-side
code itself, so that it's more obviously correct and easier to test.

Hopefully others will find it easy enough to understand and modify this test. To simplify
it a bit, you could replace some of the pairs with structs, thus reducing the use of "first"
and "second".

That being said, I think that the complexity is ok for a test, and I appreciate the effort
you put into this!

I also pointed out a few spelling mistakes.
File be/src/service/
PS10, Line 34: a
nit: an
PS10, Line 73: lb
Can you name those lower_bound and upper_bound for readability?
PS10, Line 75: T
const T&?
PS10, Line 75:  
nit: remove space
PS10, Line 76: These
I don't understand which options this comment refers to. Can you rephrase it so it is more
PS10, Line 105: accepts
nit: typo
PS10, Line 106: threat
nit: typo
PS10, Line 106: has
PS10, Line 146:  
nit: space
PS10, Line 148: auto
specify the type
PS10, Line 172:   // CASE(make_pair("prefetch_mode", &options.prefetch_mode),
This comment looks stale
PS10, Line 216: test_case.second.first
Can you wrap these in variables so it becomes easier to read? I think it's generally nice
to avoid chaining pair members.
PS10, Line 226: have

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:20:04 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message