Quanlong Huang 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 16: (6 comments) Switched to a more robust solution as Michael suggested and added more tests. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@685 PS13, Line 685: target_ids.push_back(std::to_string(target.node_id)); > We should consider switching to std::to_string, which is often more optimiz Done. It's a good chance to improve this for existing codes. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@688 PS13, Line 688: } > Comment should mention that it prints "N" when none match. I'd probably be It was discussed here: https://gerrit.cloudera.org/c/24123/4/be/src/runtime/coordinator.cc#684 I think "N/A" can't represent None so prefer "N" here. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@799 PS13, Line 799: return Substitute("\n$0", table_printer.ToString()); > This all seems a little fragile (if the output format ever changes). Could Yeah, I initially chose this solution since its scope is small and we already have similar logics in workload management that parse the profiles. Switched the solution to report effective filter targets in the backend status reports. We don't need this method now. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@818 PS13, Line 818: DCHECK(exec_status_.ok()) << exec_status_; > You already passed DCHECK(fields.size() > 1) and DCHECK(curr_scan_node_id > DCHECKs are removed in release build. So adding these as safe guards. We check counters like "Files rejected", "RowGroups rejected", "Rows rejected", "Splits rejected". Removed this method since we switch to the other approach. 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: > I actually traced this file back during my aggregated profile change: I'm fine for where to add the test. Moved this to tests/custom_cluster/test_observability.py. Hope that looks good to you. http://gerrit.cloudera.org:8080/#/c/24123/14/tests/custom_cluster/test_runtime_profile.py File tests/custom_cluster/test_runtime_profile.py: http://gerrit.cloudera.org:8080/#/c/24123/14/tests/custom_cluster/test_runtime_profile.py@51 PS14, Line 51: > flake8: W391 blank line at end of file Done -- 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: 16 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Mon, 18 May 2026 07:58:41 +0000 Gerrit-HasComments: Yes
