You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Sahil Takiar (Code Review)" <ge...@cloudera.org> on 2019/07/24 03:11:39 UTC
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Sahil Takiar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13907
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
4 files changed, 295 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/1
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/query_test/test_result_spooling.py@72
PS7, Line 72: class TestResultSpoolingCancellation(ImpalaTestSuite):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py
File tests/util/cancel_util.py:
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@79
PS7, Line 79: def __fetch_results():
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@84
PS7, Line 84: q
flake8: F821 undefined name 'query'
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@84
PS7, Line 84: h
flake8: F821 undefined name 'handle'
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 21:56:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3972/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 03:59:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/query_test/test_result_spooling.py@72
PS7, Line 72: class TestResultSpoolingCancellation(ImpalaTestSuite):
> flake8: E302 expected 2 blank lines, found 1
Done
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py
File tests/util/cancel_util.py:
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@79
PS7, Line 79: def __fetch_results():
> flake8: E302 expected 2 blank lines, found 1
Done
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@84
PS7, Line 84: q
> flake8: F821 undefined name 'query'
Done
http://gerrit.cloudera.org:8080/#/c/13907/7/tests/util/cancel_util.py@84
PS7, Line 84: h
> flake8: F821 undefined name 'handle'
Done
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 22:02:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#11).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/hs2/test_fetch_first.py
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 314 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/11
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test:
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@21
PS6, Line 21: preserved
> preserved
Done
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@62
PS6, Line 62: avg(int_col)
> May want to double check but will the following exercise the expression eva
Yeah, you are right. Just running 'avg(int_col)' does not trigger the expression eval logic, changed it to 'avg(int_col) + avg(bigint_col)' and confirmed the expression evaluation logic is invoked.
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@64
PS6, Line 64: hen result spooling is " \
: "enabled".format(query)
: assert len(result.data) == len(base_data), "{0} returned a d
> We may not be very consistent on this but some code review comments in the
Done
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@67
PS6, Line 67: "results when result spooling was " \
: "enabled".format(que
> I seem to recall that the assert will print out the values which trigger th
Yes, that is the case:
This code:
arr1 = [1, 2, 3]
arr2 = [4, 5, 6]
assert arr1 == arr2
Prints out:
assert arr1 == arr2
E assert [1, 2, 3] == [4, 5, 6]
E At index 0 diff: 1 != 4
E Full diff:
E - [1, 2, 3]
E + [4, 5, 6]
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@96
PS6, Line 96: v.get_value('table_format').compression_codec == 'none')
> Do we also add a test case with result caching enabled ?
I added some tests to test_fetch_first.py with result spooling enabled, they currently fail because of IMPALA-8819, but I added them anyway with an xfail tag so they can be enabled when IMPALA-8819 is done.
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py
File tests/util/cancel_util.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@26
PS6, Line 26:
> nit: long line
Done
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@37
PS6, Line 37: thread = threading.Thread(target=__fetch_results)
: thread.start()
:
: sleep(cancel_delay)
: assert client.get_state(handle) != client.QUERY_STATES['EXCEPTION']
: cancel_result = client.cancel(handle)
: assert cancel_result.status_code == 0,\
: 'Unexpected status code from cancel request: %s' %
> nit: not your change but it may be slightly easier to read if this function
Done
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@64
PS6, Line 64: # failed with 'Cancelled' or failed with 'Invalid query handle' (if the close
: # rpc occur
> Aren't we calling thread join twice if 'join_before_close' is True ?
Yes, but according to the docs "A thread can be join()ed many times." (https://docs.python.org/2/library/threading.html#threading.Thread.join)
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 21:57:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4718/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 15:17:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4128/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 23:39:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4120/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 22:36:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test:
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@21
PS6, Line 21: presered
preserved
http://gerrit.cloudera.org:8080/#/c/13907/6/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@62
PS6, Line 62: avg(int_col)
May want to double check but will the following exercise the expression evaluation logic more in plan root sink ?
select avg(int_col) + avg(bigint_col) from ...;
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@64
PS6, Line 64: "%s returned a different number of " \
: "results when result spooling was " \
: "enabled" % query
We may not be very consistent on this but some code review comments in the past suggested me to use format instead (similar to Substitute in our C++ code). I suppose it's not as type sensitive (%s vs %d ?).
"{0} return ...".format(query)
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@67
PS6, Line 67: "%s returned different results when result " \
: "spooling was enabled" % query
I seem to recall that the assert will print out the values which trigger the failure. Can you please double check if that's the case ? If not, we may wanna include the result sets in the error message for debugging purposes.
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/query_test/test_result_spooling.py@96
PS6, Line 96:
Do we also add a test case with result caching enabled ?
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py
File tests/util/cancel_util.py:
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@26
PS6, Line 26: The
nit: long line
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@37
PS6, Line 37: def fetch_results():
: threading.current_thread().fetch_results_error = None
: threading.current_thread().query_profile = None
: try:
: new_client = ImpalaTestSuite.create_impala_client()
: new_client.fetch(query, handle)
: except ImpalaBeeswaxException as e:
: threading.current_thread().fetch_results_error = e
nit: not your change but it may be slightly easier to read if this function is not nested
http://gerrit.cloudera.org:8080/#/c/13907/6/tests/util/cancel_util.py@64
PS6, Line 64: # Before accessing fetch_results_error we need to join the fetch thread
: thread.join()
Aren't we calling thread join twice if 'join_before_close' is True ?
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 6
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 01:26:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 21:33:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test:
http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@69
PS4, Line 69: queriess
> typo
Done
http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py@27
PS4, Line 27: 'select * from lineitem order by l_orderkey'
> I suppose this generates more than row batches, right ? Should this also be
test_multi_batches already covers that case by setting the BATCH_SIZE to 10 and running a query with a limit 100 (I changed that to limit 1000 since we buffer 10 * BATCH_SIZE).
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jul 2019 00:17:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4141/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 11
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 20:48:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#3).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
M be/src/exec/buffered-plan-root-sink.cc
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 300 insertions(+), 66 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/3
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#8).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/hs2/test_fetch_first.py
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 314 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/8
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4090/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 31 Jul 2019 00:57:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13907/1/tests/query_test/test_cancellation.py
File tests/query_test/test_cancellation.py:
http://gerrit.cloudera.org:8080/#/c/13907/1/tests/query_test/test_cancellation.py@153
PS1, Line 153: v
flake8: E126 continuation line over-indented for hanging indent
http://gerrit.cloudera.org:8080/#/c/13907/1/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/1/tests/query_test/test_result_spooling.py@38
PS1, Line 38: \
flake8: E502 the backslash is redundant between brackets
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 03:12:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#5).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
4 files changed, 295 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/5
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 5
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 10:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@26
PS10, Line 26: tpch
> tpch_parquet?
Done
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@39
PS10, Line 39: v.get_value('table_format').file_format == 'parquet')
> Do we want to run it for all file formats in exhaustive? I think it makes s
Done
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@46
PS10, Line 46: self.run_test_case('QueryTest/result-spooling', vector)
> Most or all of the queries explicitly specify "functional", which is text,
Done
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@58
PS10, Line 58: result = self.execute_query(query, exec_options)
> Maybe make a copy of exec_options, it's a little surprising that this funct
Done
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@96
PS10, Line 96: v.get_value('table_format').file_format == 'parquet' and
> The same nit applies here that you've specified parquet but are querying te
Changed all the queries to use 'tpch_parquet' so this should be fixed now.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 20:30:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 1:
This patch is a parent of https://gerrit.cloudera.org/#/c/13883/ - I will re-run the tests after https://gerrit.cloudera.org/#/c/13883/ is merged.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 03:13:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 10:
Not entirely sure why, but when test_result_spooling.py is run on Jenkins, the TPCH queries need to be prefixed with the database name (the confusing part is that test_cancellation.py doesn't need to do this). Regardless, it doesn't seem like a big deal to prefix them with 'tpch'.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 23:00:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4733/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 12
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 21:45:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 15:17:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 12
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Aug 2019 02:35:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 10:
(5 comments)
Just a few nits about the test dimensions after I saw your last comment. Specifying the database manually to match the test dimension seems fine. I forget the details, but I think it may only automatically switch databases to match the workload and dimension when running .test fiels.
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@26
PS10, Line 26: tpch
tpch_parquet?
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@39
PS10, Line 39: v.get_value('table_format').file_format == 'parquet')
Do we want to run it for all file formats in exhaustive? I think it makes sense just to run it on parquet.
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@46
PS10, Line 46: self.run_test_case('QueryTest/result-spooling', vector)
Most or all of the queries explicitly specify "functional", which is text, which doesn't line up with the test dimensions. This is kinda confusing. I'd suggest either using functional_parquet or just leaving the database off (that should work in the .test files, at least).
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@58
PS10, Line 58: result = self.execute_query(query, exec_options)
Maybe make a copy of exec_options, it's a little surprising that this function modifies its argument.
http://gerrit.cloudera.org:8080/#/c/13907/10/tests/query_test/test_result_spooling.py@96
PS10, Line 96: v.get_value('table_format').file_format == 'parquet' and
The same nit applies here that you've specified parquet but are querying text tables.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sun, 04 Aug 2019 21:25:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 12
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Aug 2019 21:45:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 9: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4718/
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 9
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 21:55:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#7).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/hs2/test_fetch_first.py
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 312 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/7
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 7
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Reviewed-on: http://gerrit.cloudera.org:8080/13907
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/hs2/test_fetch_first.py
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 314 insertions(+), 64 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 13
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Aug 2019 04:43:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#10).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/hs2/test_fetch_first.py
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
5 files changed, 314 insertions(+), 64 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/10
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 10
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4121/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 8
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Aug 2019 22:44:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3971/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 1
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 03:52:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13907
to look at the new patch set (#2).
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Adds additional tests to test_result_spooling.py to cover various edge
cases when fetching query results (ensure all Impala types are returned
properly, UDFs are evaluated correctly, etc.). A new QueryTest file
result-spooling.test is added to encapsulate all these tests. Tests with
a decreased ROW_BATCH_SIZE are added as well to validate that
BufferedPlanRootSink buffers row batches correctly.
BufferedPlanRootSink requires careful synchronization of the producer
and consumer threads, especially when queries are cancelled. The
TestResultSpoolingCancellation class is dedicated to running
cancellation tests with SPOOL_QUERY_RESULTS = true. The implementation
is heavily borrowed from test_cancellation.py and some of the logic is
re-factored into a new utility class called cancel_utils.py to avoid
code duplication between test_cancellation.py and
test_result_spooling.py.
Testing:
* Looped test_result_spooling.py overnight with no failures
* Core tests passed
Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
---
A testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
M tests/query_test/test_cancellation.py
M tests/query_test/test_result_spooling.py
A tests/util/cancel_util.py
4 files changed, 295 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13907/2
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 2
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4029/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 19:42:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8781: Result spooling tests to cover edge cases and cancellation
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13907 )
Change subject: IMPALA-8781: Result spooling tests to cover edge cases and cancellation
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test
File testdata/workloads/functional-query/queries/QueryTest/result-spooling.test:
http://gerrit.cloudera.org:8080/#/c/13907/4/testdata/workloads/functional-query/queries/QueryTest/result-spooling.test@69
PS4, Line 69: queriess
typo
http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py
File tests/query_test/test_result_spooling.py:
http://gerrit.cloudera.org:8080/#/c/13907/4/tests/query_test/test_result_spooling.py@27
PS4, Line 27: 'select * from lineitem order by l_orderkey'
I suppose this generates more than row batches, right ? Should this also be run in the context of non-cancellation test ?
--
To view, visit http://gerrit.cloudera.org:8080/13907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3b3a1539c4a5fa9b43c8ca315cea16c9701e283
Gerrit-Change-Number: 13907
Gerrit-PatchSet: 4
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jul 2019 07:25:08 +0000
Gerrit-HasComments: Yes