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 12: (16 comments) http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@59 PS11, Line 59: Enu > what's the reasoning behind the terminology "Enum" here? oh, is it that th Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@68 PS11, Line 68: > what's that? It's an outdated comment and is updated. http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@71 PS11, Line 71: > is that modified? if yes, we generally use pointer instead of reference. If Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@78 PS11, Line 78: template<typename T> > same comments Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@86 PS11, Line 86: // Create a shortcut function to test if 'str' would result into a failure. > document what this test case is verifying. Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@92 PS11, Line 92: T> > what is a "byte case"? oh, maybe it means query option that take a number Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@94 PS11, Line 94: [options, > document what that is Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@95 PS11, Line 95: ueryOption(opt > same comment as above Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@132 PS11, Line 132: : TestError("1%"); > that's the explanation I think is missing above. Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@161 PS11, Line 161: {M > explain what it means to "test" an enum. Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@161 PS11, Line 161: can_range_length), {-1, I64_MAX}} > what is that trying to convey? It tries to explain the parameter accept_integer. Now documented in params. http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@162 PS11, Line 162: {MAKE_OPTIONDEF(rm_initial_mem), {-1, I64_MAX}}, : {MAKE_OPTIONDEF(buffer_pool_limit), {-1, I64_MAX}}, : {MAKE_OPTIONDEF(max_ > document the params (include template param). Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@218 PS11, Line 218: / ENTRY(_, TPrefetch > let's be clearer about what we verify. Done http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@243 PS11, Line 243: estEnumCase(&options, CASE(compression_codec, > same I'm not sure how to clarify what we verify here. The rules are described in the comments below and there isn't much to summary. http://gerrit.cloudera.org:8080/#/c/7805/11/testdata/workloads/functional-query/queries/QueryTest/set.test File testdata/workloads/functional-query/queries/QueryTest/set.test: http://gerrit.cloudera.org:8080/#/c/7805/11/testdata/workloads/functional-query/queries/QueryTest/set.test@a145 PS11, Line 145: > does the new unit test really give this same coverage? does it verify these Error message is not verified. Do you suggest leaving them here or testing them in query-option-test.cc? If we leave them here we might add verification of other error messages as well. Testing them in query-option-test involves redesigning it completely. http://gerrit.cloudera.org:8080/#/c/7805/11/testdata/workloads/functional-query/queries/QueryTest/set.test@a293 PS11, Line 293: > same question See above. -- 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: 12 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: Mon, 02 Oct 2017 22:03:24 +0000 Gerrit-HasComments: Yes
