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 <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 01:26:40 +0000 Gerrit-HasComments: Yes
