Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16019 )
Change subject: IMPALA-9809: Multi-aggregation query on particular dataset crashes impalad ...................................................................... Patch Set 3: (3 comments) Great! The tpch-based test is a big improvement. I had a few more nits about the test, but I'm ready to +2 once those are addressed. http://gerrit.cloudera.org:8080/#/c/16019/3/testdata/workloads/functional-query/queries/QueryTest/min-multiple-distinct-aggs.test File testdata/workloads/functional-query/queries/QueryTest/min-multiple-distinct-aggs.test: http://gerrit.cloudera.org:8080/#/c/16019/3/testdata/workloads/functional-query/queries/QueryTest/min-multiple-distinct-aggs.test@8 PS3, Line 8: tpch Can you remove the tpch. prefix? It shouldn't be necessary if this is called from a test class which specifies the tpch workload. It's a good practice not to specify the database name, and let the test framework do the work (if you hardcode the db name in the .test files, it means that the file format part of the text matrix doesn't work properly) I check that the query still crashes if you run against tpch_parquet, btw. http://gerrit.cloudera.org:8080/#/c/16019/3/testdata/workloads/functional-query/queries/QueryTest/min-multiple-distinct-aggs.test@10 PS3, Line 10: ---- TYPES It'd be good to include a results section here to verify that it produces the correct results. Particularly cause crashes and incorrect results often go hand-in-hand. I guess you need to modify the query so that the results are deterministic - I don't have strong feelings about how, adding an order by including all the columns is usually fine. http://gerrit.cloudera.org:8080/#/c/16019/3/tests/query_test/test_aggregation.py File tests/query_test/test_aggregation.py: http://gerrit.cloudera.org:8080/#/c/16019/3/tests/query_test/test_aggregation.py@379 PS3, Line 379: def test_min_multiple_distinct(self, vector, unique_database): This fits better in TestTPCAggregationQueries below (l419) -- To view, visit http://gerrit.cloudera.org:8080/16019 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06d73171cdc40bdbb15960573030ac7fc94a7e16 Gerrit-Change-Number: 16019 Gerrit-PatchSet: 3 Gerrit-Owner: Yongzhi Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Yongzhi Chen <[email protected]> Gerrit-Comment-Date: Tue, 02 Jun 2020 17:29:28 +0000 Gerrit-HasComments: Yes
