Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24693 )
Change subject: IMPALA-15236: Expose HBO match provenance ...................................................................... Patch Set 2: (7 comments) Thanks for the review. PS3 addresses all of it; the one I answered rather than changed is the goldens question, below. http://gerrit.cloudera.org:8080/#/c/24693/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24693/2//COMMIT_MSG@15 PS2, Line 15: IMPALA-15127, including constant-only UNION plans. > This seems related to a rebase and can be removed. Done - dropped, and rewrote the message now that IMPALA-15127 is in master. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@487 PS2, Line 487: .append("HBO match: strategy=").append(hboStrategy_) > nit: this seems redundant since the "from HBO" annotation already shows it. Done - the details line now carries only the hash and the original estimate. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@491 PS2, Line 491: .append("\n"); > This adds one line for each PlanNode, which seems too verbose for the defau Went with the query option: ENABLE_EXPLAIN_HBO, off by default, so the default EXPLAIN text stays as it is. Gating on EXTENDED would have meant moving test_hbo.py to that level and regenerating every golden with resource estimates for the sake of three lines. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1122 PS2, Line 1122: hboHashKey_ = null; > nit: maybe encapsulate these into a record like PlanNodeStatsMatch. hasHboC Done - PlanNode keeps a single hboMatch_ record now, hasHboCard_ is gone and the annotation checks it for null. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/service/HistoricalStats.java File fe/src/main/java/org/apache/impala/service/HistoricalStats.java: http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@40 PS2, Line 40: public static final class PlanNodeStatsMatch { > nit: use record in Java 17 Done. HistoricalStatsTest moved to the record accessors along with it. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@185 PS2, Line 185: * Retrieves the number of output rows from historical stats, trying multiple hash keys > nit: this also need an update Done. http://gerrit.cloudera.org:8080/#/c/24693/2/tests/query_test/test_hbo.py File tests/query_test/test_hbo.py: http://gerrit.cloudera.org:8080/#/c/24693/2/tests/query_test/test_hbo.py@67 PS2, Line 67: if 'cardinality' in line] > Do you think we should relax this constraint to also check the HBO details The details line carries the hash key, which changes whenever the key format changes, so folding it into the golden comparison would mean regenerating every hbo-*.test on each canonicalization tweak. That is why the provenance is checked in test_matching_provenance instead, where the hash is matched by shape rather than by value, and the goldens keep comparing cardinality lines only. Happy to widen the filter if you would rather have it there. -- To view, visit http://gerrit.cloudera.org:8080/24693 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d2deaaf78a2a41353454ba634a247d8c69825bf Gerrit-Change-Number: 24693 Gerrit-PatchSet: 2 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 21 Aug 2026 07:38:23 +0000 Gerrit-HasComments: Yes
