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 <tw...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 30 Sep 2017 00:20:04 +0000 Gerrit-HasComments: Yes
