impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tianyi Wang (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/7805 )

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


Patch Set 10:

(12 comments)

> Patch Set 10: Code-Review+1
> 
> (13 comments)
> 
> 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.

http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@34
PS10, Line 34: a
> nit: an
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@73
PS10, Line 73: lb
> Can you name those lower_bound and upper_bound for readability?
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@75
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.


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@75
PS10, Line 75:  
> nit: remove space
Done.


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@76
PS10, Line 76: These
> I don't understand which options this comment refers to. Can you rephrase i
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@105
PS10, Line 105: accepts
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@106
PS10, Line 106: threat
> nit: typo
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@106
PS10, Line 106: has
> have
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@146
PS10, Line 146:  
> nit: space
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@148
PS10, Line 148: auto
> specify the type
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@172
PS10, Line 172:   // CASE(make_pair("prefetch_mode", &options.prefetch_mode),
> This comment looks stale
Done


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@226
PS10, Line 226: have
> has
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <twang@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Sat, 30 Sep 2017 01:57:29 +0000
Gerrit-HasComments: Yes

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