impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (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 00:20:04 GMT
Lars Volker 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: 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.

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


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?


http://gerrit.cloudera.org:8080/#/c/7805/10/be/src/service/query-options-test.cc@75
PS10, Line 75: T
const T&?


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


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 it so it is more
clear?


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


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


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


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


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


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


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


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



-- 
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: 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 00:20:04 +0000
Gerrit-HasComments: Yes

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