[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

Dan Hecht 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 11:

(16 comments)

I agree this is generally pretty tough to read. We wouldn't want to add so much metaprogramming to impala, but I'm okay with moving forward with it if we keep it to the unit test, provided we clarify the comments (which are extra important in this case).

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: Enum
what's the reasoning behind the terminology "Enum" here?  oh, is it that this is only used to test query options that take an enumeration for possible values? let's be more explicit.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@68
PS11, Line 68: 'value'
what's that?


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@71
PS11, Line 71: TQueryOptions& options
is that modified? if yes, we generally use pointer instead of reference. If not, then "const ref" is okay. also document these params


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@78
PS11, Line 78: // Create a shortcut function to test if 'value' would result into a failure.
same comments


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@86
PS11, Line 86: TEST(QueryOptions, SetNonExistentOptions) {
document what this test case is verifying.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@92
PS11, Line 92: byte cases
what is a "byte case"?  oh, maybe it means query option that take a number of bytes as input. let's be explicit about this.
and what does it mean to "test a set" of them?  please summarize what we verify.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@94
PS11, Line 94: typename T
document what that is


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@95
PS11, Line 95: TQueryOptions&
same comment as above


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@132
PS11, Line 132: Byte size options accept integers and integers with a
              : // suffix like "KB".
that's the explanation I think is missing above.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@161
PS11, Line 161: Test 
explain what it means to "test" an enum.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@161
PS11, Line 161: It may or may not accept integers
what is that trying to convey?


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@162
PS11, Line 162: template<typename T>
              : void TestEnumCase(TQueryOptions& options, const EnumCase<T>& test_case,
              :     bool accept_integer) {
document the params (include template param).


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@218
PS11, Line 218: Test integer options
let's be clearer about what we verify.


http://gerrit.cloudera.org:8080/#/c/7805/11/be/src/service/query-options-test.cc@243
PS11, Line 243: Test options with non regular validation rule.
same


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 messages?


http://gerrit.cloudera.org:8080/#/c/7805/11/testdata/workloads/functional-query/queries/QueryTest/set.test@a293
PS11, Line 293: 
same question



-- 
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: 11
Gerrit-Owner: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Oct 2017 17:00:18 +0000
Gerrit-HasComments: Yes