impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options
Date Mon, 02 Oct 2017 17:00:18 GMT
Dan Hecht has posted comments on this change. ( )

Change subject: IMPALA-5425: Add test for validating input when setting query options

Patch Set 11:


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).
File be/src/service/
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.
PS11, Line 68: 'value'
what's that?
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
PS11, Line 78: // Create a shortcut function to test if 'value' would result into a failure.
same comments
PS11, Line 86: TEST(QueryOptions, SetNonExistentOptions) {
document what this test case is verifying.
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.
PS11, Line 94: typename T
document what that is
PS11, Line 95: TQueryOptions&
same comment as above
PS11, Line 132: Byte size options accept integers and integers with a
              : // suffix like "KB".
that's the explanation I think is missing above.
PS11, Line 161: Test 
explain what it means to "test" an enum.
PS11, Line 161: It may or may not accept integers
what is that trying to convey?
PS11, Line 162: template<typename T>
              : void TestEnumCase(TQueryOptions& options, const EnumCase<T>&
              :     bool accept_integer) {
document the params (include template param).
PS11, Line 218: Test integer options
let's be clearer about what we verify.
PS11, Line 243: Test options with non regular validation rule.
File testdata/workloads/functional-query/queries/QueryTest/set.test:
PS11, Line 145: 
does the new unit test really give this same coverage? does it verify these error messages?
PS11, Line 293: 
same question

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Tianyi Wang <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Mon, 02 Oct 2017 17:00:18 +0000
Gerrit-HasComments: Yes

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