Quanlong Huang 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) The details look pretty good! http://gerrit.cloudera.org:8080/#/c/24693/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24693/2//COMMIT_MSG@14 PS2, Line 14: Keep the empty-scan matching introduced by : IMPALA-15127, including constant-only UNION plans. This seems related to a rebase and can be removed. 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: hboStrategy_ nit: this seems redundant since the "from HBO" annotation already shows it. 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 default explain_level. It also changes the user-visible EXPLAIN text that external tooling may parse. Can we only add it when explain_level is set to >= 2, i.e. EXTENDED or VERBOSE? Or we can add a new query option like ENABLE_EXPLAIN_HBO (similar to ENABLE_EXPLAIN_CALCITE) to control whether to show these details. A query option seems more friendly for testing these lines. http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1120 PS2, Line 1120: cardinalityBeforeHbo_ = -1; : hboStrategy_ = null; : hboHashKey_ = null; nit: maybe encapsulate these into a record like PlanNodeStatsMatch. hasHboCard_ can then be replaced by checking if this record isn't 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 http://gerrit.cloudera.org:8080/#/c/24693/2/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@185 PS2, Line 185: the number of output rows nit: this also need an update 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@63 PS2, Line 63: # To avoid the test being fragile, we only compare the cardinality lines. : actual_cardinality_lines = [line for line in actual_plan_lines : if 'cardinality' in line] : expected_cardinality_lines = [line for line in expected_plan_lines : if 'cardinality' in line] Do you think we should relax this constraint to also check the HBO details lines? -- 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: Thu, 20 Aug 2026 14:11:46 +0000 Gerrit-HasComments: Yes
