Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/23154 )
Change subject: IMPALA-9846: Enable AGGREGATED PROFILE by Default ...................................................................... Patch Set 33: (17 comments) Thank you for the review and the helpful comments! I have updated the patchset based on your feedback. Specifically, I have switched back to the traditional profile and reverted the associated Python test changes wherever instance-level details were strictly necessary. Additionally, to improve the readability and reliability of the test cases, I have replaced the complex regex expressions with the 'aggregation(SUM, expr)' approach. Regarding these newly added aggregation changes, a test dry run is required to capture and update the actual SUM values, as I do not have all the necessary table data locally. I have already triggered this dry run to identify the failing tests and will update the exact values as soon as it finishes. However, these pending numerical updates do not impact the underlying logic and should not hinder the ongoing code review. Thanks again for your time and feedback! http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test File testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test: http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test@324 PS32, Line 324: aggregation(SUM, InMemoryHeapsEvicted): 0 > This is a good example for improving the test in the patch, as the conditio Done http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test File testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test: http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test@543 PS32, Line 543: Partition: year=2010 /day=3 > Is this connected somehow to the current change? Done http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/compute-stats-incremental.test@543 PS32, Line 543: Partition: year=2010 /day=3 > Is this connected somehow to the current change? Done http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/nested-types-scanner-array-materialization.test File testdata/workloads/functional-query/queries/QueryTest/nested-types-scanner-array-materialization.test: http://gerrit.cloudera.org:8080/#/c/23154/32/testdata/workloads/functional-query/queries/QueryTest/nested-types-scanner-array-materialization.test@19 PS32, Line 19: ---- RUNTIME_PROFILE > This type of rewrite was discussed at several places. Done http://gerrit.cloudera.org:8080/#/c/23154/30/tests/custom_cluster/test_query_live.py File tests/custom_cluster/test_query_live.py: http://gerrit.cloudera.org:8080/#/c/23154/30/tests/custom_cluster/test_query_live.py@521 PS30, Line 521: : # impala_query_live is assigned to build side, so executor has HDFS > These checks don't make sense anymore. Done http://gerrit.cloudera.org:8080/#/c/23154/30/tests/custom_cluster/test_tuple_cache.py File tests/custom_cluster/test_tuple_cache.py: http://gerrit.cloudera.org:8080/#/c/23154/30/tests/custom_cluster/test_tuple_cache.py@63 PS30, Line 63: profile, key) > What does total/mean mean here in the non-aggregated case? Done http://gerrit.cloudera.org:8080/#/c/23154/30/tests/custom_cluster/test_tuple_cache.py@534 PS30, Line 534: but d > This makes the test weaker by allowing both numbers in both aggregated and Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_aggregation.py File tests/query_test/test_aggregation.py: http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_aggregation.py@166 PS32, Line 166: 1, 2, 4, > I don't understand this change - these are supposed to be node ids, if the Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_hash_join_timer.py File tests/query_test/test_hash_join_timer.py: http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_hash_join_timer.py@156 PS32, Line 156: assert (asyn_build), "Join is not prepared asynchronously: {0}".format(profile) : assert (check_fragment_count > 1), \ : "Unable to verify Fragment or Average Fragment: {0}".format(profile) : : def __verify_join_time(self, duration_ms, comme > This does not verify in the aggregated case what the assert is about. Done http://gerrit.cloudera.org:8080/#/c/23154/30/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/23154/30/tests/query_test/test_iceberg.py@1265 PS30, Line 1265: def test_scheduling_partitioned_tables(self, unique_d > This looks wrong - the next loop checks the differences against the avg, wh Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_iceberg.py@1306 PS32, Line 1306: + \((\d+) > Why is this needed if the profile is not aggregated? Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_iceberg.py@1307 PS32, Line 1307: avg_files_rejected > The semantic seems changed with aggregated profile - shouldn't it look for Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py@434 PS32, Line 434: rn count > This changes the semantics of the test. The loop at line 464 sums the numbe Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py@434 PS32, Line 434: rn count > This changes the semantics of the test. The loop at line 464 sums the numbe Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py@1449 PS32, Line 1449: fq_table_name = db_name + '.' + table_name > This no longer checks the original intention of the test. The goal was to c Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py@1747 PS32, Line 1747: > Why 3? Won't this also match the average instance? Done http://gerrit.cloudera.org:8080/#/c/23154/32/tests/query_test/test_scanners.py@1747 PS32, Line 1747: > Why 3? Won't this also match the average instance? Done -- To view, visit http://gerrit.cloudera.org:8080/23154 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If41d6322361fba82c946efd614cc7d28cb1c36e8 Gerrit-Change-Number: 23154 Gerrit-PatchSet: 33 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 19 Aug 2026 13:33:37 +0000 Gerrit-HasComments: Yes
