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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 15: Verified+1


--
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: 15
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 10 Oct 2017 21:54:32 +
Gerrit-HasComments: No

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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/7805 )

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Reviewed-on: http://gerrit.cloudera.org:8080/7805
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 267 insertions(+), 128 deletions(-)

Approvals:
  Dan Hecht: Looks good to me, approved
  Impala Public Jenkins: Verified

--
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: merged
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 16
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins 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 15:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1324/


--
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: 15
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Tue, 10 Oct 2017 17:54:15 +
Gerrit-HasComments: No

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

[Dan Hecht (Code Review)]

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 15: Code-Review+2


--
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: 15
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: Tue, 10 Oct 2017 17:54:09 +
Gerrit-HasComments: No

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

[Lars Volker (Code Review)]

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 14: Code-Review+2

This has 4 +1's now, I'm aggregating them into a +2.


--
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: 14
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, 09 Oct 2017 23:11:00 +
Gerrit-HasComments: No

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

[Tianyi Wang (Code Review)]

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 14:

> Uploaded patch set 14.

Rebase


--
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: 14
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: Fri, 06 Oct 2017 21:00:36 +
Gerrit-HasComments: No

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

[Tianyi Wang (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/7805

to look at the new patch set (#14).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 267 insertions(+), 128 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/14
--
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: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 14
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Dan Hecht (Code Review)]

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 13: Code-Review+1

(1 comment)

Like I said, let's avoid this much indirection/metaprogramming in general, but 
for this unit test I'm okay with it.  Alex, can you also take a look to see if 
this satisfies what you wanted IMPALA-5425 to cover? (if no, we could go 
forward with the change as is and do another change, if that makes sense).

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:
> Tests involving error messages are restored. I don't understand what "gener
yeah, that's what I was asking -- whether checking one of each class of option 
types is sufficient or if each needs to be checked. sounds like for bytesize, 
one option is pretty good, but for some other ones, there is custom logic.



--
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: 13
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: Tue, 03 Oct 2017 17:21:22 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/7805

to look at the new patch set (#13).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 301 insertions(+), 127 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/13
--
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: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 13
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Dan Hecht (Code Review)]

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:

(1 comment)

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:
> Error message is not verified. Do you suggest leaving them here or testing 
Maybe we should leave these then.  I forget, are these generic messages 
produced by the query option code itself, or specific to each individual option?



--
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 
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 22:22:20 +
Gerrit-HasComments: Yes

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

[Dan Hecht (Code Review)]

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 12:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7805/12/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/7805/12/be/src/service/query-options-test.cc@70
PS12, Line 70: if 'str' would be successfully read as expected
 : // value.
I don't understand that. Maybe this should say:

Create a shortcut function to verify that the given query option has the 
'expected_value' value after setting the 'option_def' option to 'str'.

or something like that?


http://gerrit.cloudera.org:8080/#/c/7805/12/be/src/service/query-options-test.cc@86
PS12, Line 86: // Create a shortcut function to test if 'str' would result into 
a failure.
I think that should be reworded as per 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 
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 22:20:44 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

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
> 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(, 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 
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 22:03:24 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#12). ( 
http://gerrit.cloudera.org:8080/7805 )

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 301 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/12
--
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: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 12
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Dan Hecht (Code Review)]

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).

The naming of fields using structs rather than pair<> definitely helped, though.


--
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 
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:01:27 +
Gerrit-HasComments: No

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

[Dan Hecht (Code Review)]

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
  : void TestEnumCase(TQueryOptions& options, const EnumCase& 
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 
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 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

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

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

[Tianyi Wang (Code Review)]

Hello Lars Volker, Tim Armstrong, Alex Behm, Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/7805

to look at the new patch set (#11).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 268 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/11
--
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: newpatchset
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 

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

[Lars Volker (Code Review)]

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:

I'd like to give Dan and Alex the chance to have a look, too.


--
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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:20:31 +
Gerrit-HasComments: No

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

[Tianyi Wang (Code Review)]

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 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7805/9/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/7805/9/be/src/service/query-options-test.cc@20
PS9, Line 20: #include 
> nit: boost includes should go before standard library includes.
Removed redundant imports.



--
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: 9
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:45:20 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Hello Lars Volker, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/7805

to look at the new patch set (#10).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 248 insertions(+), 154 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/10
--
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: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Tim Armstrong (Code Review)]

Tim Armstrong 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 9: Code-Review+1

(1 comment)

Looks good to me. I think Lars may have still had some questions or feedback so 
I'll let him +2

http://gerrit.cloudera.org:8080/#/c/7805/9/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

http://gerrit.cloudera.org:8080/#/c/7805/9/be/src/service/query-options-test.cc@20
PS9, Line 20: #include 
nit: boost includes should go before standard library includes.



--
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: 9
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:18:22 +
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7805/8/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS8, Line 67: SetNonExistOptions)
> SetNonExistentOptions.
Done


Line 124:   {MAKE_KEYDEF(max_row_size),  {1, I64_MAX}},
> Should add DEFAULT_SPILLABLE_BUFFER_SIZE and MIN_SPILLABLE_BUFFER_SIZE. Tho
They are in SetSpecialOptions.


Line 175:   // ENTRIES(TPrefetchMode, (None)(HT_BUCKET)) expands to
> Can you add blank lines after the macro definitions? It's a bit hard to see
Done


PS8, Line 219: err
> It might be clearer to call these functions something like TestError() and 
Done.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#9).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 251 insertions(+), 153 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 8:

(4 comments)

I'm generally ok with this patch. I think there are places where there's a 
trade-off between using macros/lambdas and repeating ourselves. My general 
feeling is that there wouldn't be a net improvement to the code by changing 
that, but I'm ok with whatever Lars and Tianyi figure out.

http://gerrit.cloudera.org:8080/#/c/7805/8/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS8, Line 67: SetNonExistOptions)
SetNonExistentOptions.


Line 124:   {MAKE_KEYDEF(max_row_size),  {1, I64_MAX}},
Should add DEFAULT_SPILLABLE_BUFFER_SIZE and MIN_SPILLABLE_BUFFER_SIZE. Those 
might require some additional logic since they're restricted to power-of-twos.


Line 175:   // ENTRIES(TPrefetchMode, (None)(HT_BUCKET)) expands to
Can you add blank lines after the macro definitions? It's a bit hard to see 
which comment goes with which macro currently.


PS8, Line 219: err
It might be clearer to call these functions something like TestError() and 
TestOk(). I don't feel strongly though - this seems readable enough to me.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 7:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS7, Line 39: i32_max
> Constants should be upper case, i.e. I32_MAX.
Done


PS7, Line 49: MakeOk
> Maybe MakeOkTest or MakeTestOkFn? It would make it clearer that this return
Done


PS7, Line 65: KEY
> MAKE_KEYDEF seems clearer to me.
Done


Line 81: auto lb = test_case.second.first;
> No. Will fix.
Done


PS7, Line 90: uint64_t
> Use static_cast()
Done


Line 103: for (auto& value : common_value) {
> Will fix.
Done


PS7, Line 182: Entry
> ENTRY
Done


PS7, Line 184: Entries
> ENTRIES
Done


Line 198:   auto enum_and_int = fusion::make_tuple(
> Does std::make_tuple not work here?
It didn't though boost claims it should.


Line 198:   auto enum_and_int = fusion::make_tuple(
> Could we avoid the use of tuples here if we changed TestEnumCaseSet to just
Done.


Line 306: #undef KEY
> It seems ok to leave this macro defined for the rest of the file.
Removed.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#8).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 248 insertions(+), 150 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 

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

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 7:

The really nice thing about the patch is that all the tests are all 
table-driven so adding query options should just require adding entries to 
tables describing the valid values of each option (this also helps enforce that 
behaviour of query options is consistent).I think we should preserve that 
aspect regardless.

>From what I can see the use of templates is directly in service of supporting 
>table-driven tests without copying and pasting the same code for every type 
>(which would be particularly painful for the enums). I think there are some 
>opportunities to remove indirection at the cost of repetition. E.g. if we 
>think people will be confused by functions returning functions we could avoid 
>that, but it makes sense to me and avoids a lot of repetition in the test 
>cases themselves.

-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

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

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 198:   auto enum_and_int = fusion::make_tuple(
> Does std::make_tuple not work here?
Could we avoid the use of tuples here if we changed TestEnumCaseSet to just run 
a single test at a time? E.g.

  TestEnumCaseSet(MAKE_KEYDEF(prefetchmode, TPrefetchMode, (NONE, HT_BUCKET));
  TestEnumCaseSet(...);


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

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


Patch Set 7:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS7, Line 39: i32_max
Constants should be upper case, i.e. I32_MAX.


PS7, Line 49: MakeOk
Maybe MakeOkTest or MakeTestOkFn? It would make it clearer that this returns a 
function.


PS7, Line 65: KEY
MAKE_KEYDEF seems clearer to me.


PS7, Line 90: uint64_t
Use static_cast()


PS7, Line 182: Entry
ENTRY


PS7, Line 184: Entries
ENTRIES


Line 198:   auto enum_and_int = fusion::make_tuple(
Does std::make_tuple not work here?


Line 306: #undef KEY
It seems ok to leave this macro defined for the rest of the file.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
> I thought that every CASE would be a parameter value, stored as a struct, t
I don't understand. Again for the example on that gtest page:

class FooTest : public ::testing::TestWithParam 

What should "???" be?


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

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


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> Do you mean replacing the make_pair in L142 or replacing the make_tuple in 
See my reply below.


Line 171:   });
> I'm not sure how these are related. I think value-parameterized-tests is us
I thought that every CASE would be a parameter value, stored as a struct, 
templatized if necessary.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 7:

(5 comments)

Thanks for the detailed review. I'd like to discuss in person if possible.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> Can you use a templatiezed class to store the test cases instead of a tuple
Do you mean replacing the make_pair in L142 or replacing the make_tuple in L148?


Line 171:   });
> Could you use https://github.com/google/googletest/blob/master/googletest/d
I'm not sure how these are related. I think value-parameterized-tests is used 
when you want to run a test multiple times, with a variable being different 
values in each run. Here it is about iterating through test cases (tuples for 
now) with different types.

To be more specific, in the example given:
class FooTest : public ::testing::TestWithParam 

What should "const char*" be replaced with here?


http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 79: auto ok = MakeOk(options, test_case.first);
> I think it would be helpful to see that these are functions here. Can you r
It is a lambda with no explicit typename. It could be replaced with a functor 
class though. I think to make it more like a function we can use CamelCase. I 
checked google C++ guide and didn't find a naming convention for lambda 
expressions.


Line 81: auto lb = test_case.second.first;
> Do these need to be auto?
No. Will fix.


Line 103: for (auto& value : common_value) {
> const? Please check elsewhere, too.
Will fix.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

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


Patch Set 7:

(5 comments)

I'm still trying to see if there are ways to simplify the code with patterns we 
use more commonly. If you disagree, let's catch up in person, since I'm not set 
on what's the right way forward here.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> I'm not sure which part I should look at. The complication here is that I n
Can you use a templatiezed class to store the test cases instead of a tuple? 
Additionally you could use a small factory method to hide the macros.


Line 171:   });
> Not that easy. Let me explain:
Could you use 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests
 with a templatized parameter class to achieve the same?


http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 79: auto ok = MakeOk(options, test_case.first);
I think it would be helpful to see that these are functions here. Can you 
remove the auto?


Line 81: auto lb = test_case.second.first;
Do these need to be auto?


Line 103: for (auto& value : common_value) {
const? Please check elsewhere, too.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 20: #include 
> We are generally trying to reduce our dependencies on Boost libraries. Can 
See comment on line 171.


Line 39: constexpr auto i32_max = numeric_limits::max();
> We currently use auto only for iterators and const& in loops.
Removed some autos.


Line 73: TEST(QueryOptions, SetByteOptions) {
> Can you add a comment explaining what the test does overall? Same for the o
Done


Line 96:   auto process_case_set = [&](auto& case_set) {
> I find this part hard to follow and its use of lambdas seems to deviate fro
Flattened them into template functions.


PS6, Line 97: kase
> Can you think of a better name?
Done


PS6, Line 99: minus_1_to_0
> Why is this needed? Is the added level of indirection worth it?
Removed.


PS6, Line 137:   // Expands to {"entry", prefix::entry},
 : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), 
prefix::entry},
 :   // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", 
prefix::b},
 : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, 
prefix, name)
 :   // A case is a pair: (keydef, [(enum_string, enum_value)])
 : #define CASE(key, enumtype, enums) make_pair(key, \
 :   vector\
 :   {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))})
> I find this very hard to understand. Can you have a look at the Google Test
I'm not sure which part I should look at. The complication here is that I need 
a test case to include:
1. the string name of that option "prefetch_mode"
2. A reference to the member in TQueryOptions: _mode
3. A list of enum value, and their string representation: 
   vector<...>{make_pair("NONE", TPrefetchMode::NONE), make_pair("HT_BUCKET", 
TPrefetchMode::HT_BUCKET)}

I added an example to show what expanded code looks like. If no macros are used 
here, this part of code would be really long and repetitive. I think though the 
macros are complicated, the test cases are clear and easy to modify. By looking 
at "CASE(KEY(prefetch_mode), TPrefetchMode, (NONE, HT_BUCKET))" it's easy to 
know how to add/modify a test case.


Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
> Can you use C++11 for_each instead?
Not that easy. Let me explain:
The complication here is that, each enum query option is of different type. For 
example, the type of options.prefetch_mode is TPrefetchMode::type. So the 
enum_and_int is actually a tuple of different types, and that's why I used 
make_tuple instead of a vector. What we need here is to iterate through the 
tuple, and that is not what std::for_each can do.


Line 263: for (auto& key_def : {key_def_min, key_def_default}) {
> const auto&?
Done.


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#7).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 267 insertions(+), 153 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 

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

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

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


Patch Set 6:

(9 comments)

Thank you for working on this. I left some comments inline. Overall I feel that 
we're still not completely decided on which new features of C++11 and new 
paradigms they allow we want to use. I'd be happy to follow up on this on dev@.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 20: #include 
We are generally trying to reduce our dependencies on Boost libraries. Can you 
replace some of these with C++11 equivalents (e.g. for_each))?


Line 39: constexpr auto i32_max = numeric_limits::max();
We currently use auto only for iterators and const& in loops.


Line 73: TEST(QueryOptions, SetByteOptions) {
Can you add a comment explaining what the test does overall? Same for the other 
tests.


Line 96:   auto process_case_set = [&](auto& case_set) {
I find this part hard to follow and its use of lambdas seems to deviate from 
our current best practices somewhat. Can you try to refactor some parts using 
plain functions to generate context objects instead?


PS6, Line 97: kase
Can you think of a better name?


PS6, Line 99: minus_1_to_0
Why is this needed? Is the added level of indirection worth it?


PS6, Line 137:   // Expands to {"entry", prefix::entry},
 : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), 
prefix::entry},
 :   // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", 
prefix::b},
 : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, 
prefix, name)
 :   // A case is a pair: (keydef, [(enum_string, enum_value)])
 : #define CASE(key, enumtype, enums) make_pair(key, \
 :   vector\
 :   {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))})
I find this very hard to understand. Can you have a look at the Google Test 
documentation on how to define test cases?


Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
Can you use C++11 for_each instead?


Line 263: for (auto& key_def : {key_def_min, key_def_default}) {
const auto&?


-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

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

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

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


Patch Set 6:

> Is this ready for review?

Yes.

-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

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

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

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


Patch Set 6:

Is this ready for review?

-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#6).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 236 insertions(+), 150 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#5).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 236 insertions(+), 150 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#4).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 233 insertions(+), 148 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#3).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 229 insertions(+), 148 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#2).

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

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

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 230 insertions(+), 148 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has restored this change.

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


Restored

Move the test to backend

-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: restore
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has abandoned this change.

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


Abandoned

I will move them to backend tests

-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 

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

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7805

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

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

This patch adds a test case file
testdata/workloads/functional-query/queries/QueryTest/query_options_validation.test
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
and
be/src/service/query-options-test.cc
This patch also fixes a bug in generating error message string for one
query option.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
A 
testdata/workloads/functional-query/queries/QueryTest/query_options_validation.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/query_test/test_query_opts.py
5 files changed, 360 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/05/7805/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7805
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang