Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24123 )
Change subject: IMPALA-14796: Show effective runtime filter targets in profile ...................................................................... Patch Set 13: Code-Review+1 (3 comments) Thank you for the updates! I've just left one final thought/observation regarding the custom cluster test placement based on the history of that test file. Everything else looks perfectly good to me. Thanks again for this fantastic work! http://gerrit.cloudera.org:8080/#/c/24123/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/24123/4/be/src/runtime/coordinator.cc@797 PS4, Line 797: odeId curr_scan_node_id = -1; > Good point. Changed to use std::string_view. Done http://gerrit.cloudera.org:8080/#/c/24123/4/be/src/runtime/coordinator.cc@815 PS4, Line 815: > Good point. Changed to use std::from_chars. Done http://gerrit.cloudera.org:8080/#/c/24123/4/tests/custom_cluster/test_runtime_profile.py File tests/custom_cluster/test_runtime_profile.py: http://gerrit.cloudera.org:8080/#/c/24123/4/tests/custom_cluster/test_runtime_profile.py@54 PS4, Line 54: test_tpch_q5_final_filter_table > I don't think this python test file is for verifying the runtime profile ge I actually traced this file back during my aggregated profile change: https://gerrit.cloudera.org/#/c/23154/30/testdata/workloads/tpch/queries/runtime-profile-aggregated.test https://github.com/apache/impala/commit/9429bd779de986d3e61858bef7e258bd73a2cacd#diff-83d65aa5f7ecfa1615da9b6be3d16e13db4c875137b1380ec3d793161b7b85a6 Both test_runtime_profile.py and runtime-profile-aggregated.test were originally added to test and verify the non-default aggregated profile and its syntax, which deviates from the default profile. I see that since the code iterates over the query profile object for the effective runtime filters, it is important to run it with the aggregated profile enabled. However, if we accumulate specific content tests here just because they need to be verified against the aggregated profile, over time the file might become bloated and deviate from its initial intended purpose. Ideally, adding such tests within related, specific custom cluster test files (similar to how it has been done in test_query_log.py or test_executor_groups.py) would be better. That being said, if we currently cannot find a relevant custom cluster test file and adding a new one would be too much overhead, keeping it here is perfectly fine for now. -- To view, visit http://gerrit.cloudera.org:8080/24123 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iccf4b87ac4579a70273f3306ec7b58850f06b17c Gerrit-Change-Number: 24123 Gerrit-PatchSet: 13 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 13 May 2026 12:40:16 +0000 Gerrit-HasComments: Yes
