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