Michael Smith 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: (4 comments) 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: effective_target_ids.push_back(lexical_cast<string>(node_id)); We should consider switching to std::to_string, which is often more optimized. Doesn't need to be in this patch. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@688 PS13, Line 688: row.push_back(effective_target_ids.empty() ? "N" : join(effective_target_ids, ", ")); Comment should mention that it prints "N" when none match. I'd probably be in favor of "-" or "N/A" (used later) instead to represent no effective nodes. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@799 PS13, Line 799: std::function<void(RuntimeProfileBase*)> find_effective_filters = This all seems a little fragile (if the output format ever changes). Could we add additional return fields for the status response instead? It's actually a little weird we're returning formatted strings like this rather than the raw values and letting the coordinator format them. http://gerrit.cloudera.org:8080/#/c/24123/13/be/src/runtime/coordinator.cc@818 PS13, Line 818: if (effective && fields.size() > 1 && curr_scan_node_id >= 0) { You already passed DCHECK(fields.size() > 1) and DCHECK(curr_scan_node_id >= 0), so everything except if(effective) is redundant. I'm not quite clear what's going on here though; can you expand on the format around " rejected" in a code comment? I guess that's something like "rows rejected" and you're looking for non-zero entries. -- 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: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Wed, 13 May 2026 21:02:05 +0000 Gerrit-HasComments: Yes
