impala-reviews mailing list archives

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

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

Patch Set 10:


> 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.

Thanks for the review.
Most of the pairs is removed in the latest patch. I hope it's easier to read now.
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&?
In the next line there is an assignment `if (boundary == -1) boundary = 0;` so it cannot be
a reference here.
PS10, Line 75:  
> nit: remove space
PS10, Line 76: These
> I don't understand which options this comment refers to. Can you rephrase i
PS10, Line 105: accepts
> nit: typo
PS10, Line 106: threat
> nit: typo
PS10, Line 106: has
> have
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 226: have
> has

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: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Sat, 30 Sep 2017 01:57:29 +0000
Gerrit-HasComments: Yes

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