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 <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Thu, 01 Aug 2019 21:57:47 +0000 Gerrit-HasComments: Yes
