Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24693 )
Change subject: IMPALA-15236: Expose HBO match provenance ...................................................................... Patch Set 4: (6 comments) PS4 addresses the two mechanical points. The accuracy rate, the profile-vs-EXPLAIN question and the unavailable case are answered inline rather than changed, since each of them is your call. http://gerrit.cloudera.org:8080/#/c/24693/4/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/24693/4/be/src/service/query-options.h@54 PS4, Line 54: TImpalaQueryOptions::ENABLE_EXPLAIN_HBO + 1; Fixed in PS4. The enum runs 0..206 with no gaps, so _TImpalaQueryOptions_VALUES_TO_NAMES.size() is 207 while NUM_QUERY_OPTIONS still said 206 - the DCHECK would have fired on any debug build. http://gerrit.cloudera.org:8080/#/c/24693/4/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/24693/4/common/thrift/Query.thrift@837 PS4, Line 837: 207: optional bool enable_explain_hbo = false Dropped in PS4. store_hbo_stats and aggregated_profile above it have no semicolon either; cte_threshold does, and that is the line I copied. http://gerrit.cloudera.org:8080/#/c/24693/4/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/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@490 PS4, Line 490: .append(PrintUtils.printEstCardinality(cardinalityBeforeHbo_)) Makes sense for tooling, and I would rather check the shape with you than pick one. As written the value goes negative once HBO more than doubles the estimate - cardinalityBeforeHbo_=100 against cardinality_=1000 gives (1 - 900/100) * 100 = -800% - and it divides by zero when the original estimate is 0, which is a real value here rather than the -1 sentinel. A few ways out: clamp at 0 so anything past 2x reads "0%"; or report the ratio cardinality_ / cardinalityBeforeHbo_ instead, which stays finite and says how far HBO moved the estimate in which direction; or keep your formula and special-case 0 alongside -1 as "N/A". The ratio reads better to me for the low-accuracy nodes you want to find, but the formula is yours - tell me which and I will put it in. http://gerrit.cloudera.org:8080/#/c/24693/4/tests/query_test/test_hbo.py File tests/query_test/test_hbo.py: http://gerrit.cloudera.org:8080/#/c/24693/4/tests/query_test/test_hbo.py@67 PS4, Line 67: if 'cardinality' in line] Leaving the goldens on the cardinality lines then. http://gerrit.cloudera.org:8080/#/c/24693/4/tests/query_test/test_hbo.py@92 PS4, Line 92: exact_query, {'explain_level': 2, 'enable_explain_hbo': True}) They affect both, and that is worth fixing if you would rather the profile always carried the details. The profile and EXPLAIN share PlanNode.getExplainString(), and the option is checked inside it, so the line is gated in both. Detail level cannot separate them: the profile asks for VERBOSE, but so does EXPLAIN with explain_level=3, and gating on that would put the line into the EXPLAIN text the option exists to keep stable. So it needs a signal from the caller - an explicit flag on the profile path, or reading enable_explain_hbo only when the string is built for EXPLAIN. Happy to do that in the next patch set if you would like the profile unconditional. http://gerrit.cloudera.org:8080/#/c/24693/4/tests/query_test/test_hbo.py@106 PS4, Line 106: r'HBO match: hash=[0-9a-f]{32}, original estimate=(?!unavailable)\S+', Yes, when the planner had no estimate of its own: printEstCardinality() renders -1 as "unavailable", and cardinalityBeforeHbo_ stays -1 when nothing was computed before the HBO lookup. The negative lookahead pins that the exact-match case does have one, so a regression that lost the original estimate would not pass silently. That is the same case your "N/A" suggestion covers, so if the accuracy rate lands this assertion should probably move to it. -- 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: 4 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: Sun, 23 Aug 2026 10:33:59 +0000 Gerrit-HasComments: Yes
