Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24693 )
Change subject: IMPALA-15236: Expose HBO match provenance ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/24693/7/fe/src/main/java/org/apache/impala/planner/CanonicalizationStrategy.java File fe/src/main/java/org/apache/impala/planner/CanonicalizationStrategy.java: http://gerrit.cloudera.org:8080/#/c/24693/7/fe/src/main/java/org/apache/impala/planner/CanonicalizationStrategy.java@68 PS7, Line 68: carries none > nit: isn't it EXPR_REWRITE? Yes - naming it now instead of describing it. http://gerrit.cloudera.org:8080/#/c/24693/7/fe/src/main/java/org/apache/impala/planner/CanonicalizationStrategy.java@69 PS7, Line 69: data > nit: IMO, the strategy controls the HBO key string which matches queries, e You're right, and the sentence had the strategy standing in for the data. Reworked: EXPR_REWRITE matches an equivalent query and so needs no caveat, IGNORE_PARTITION_CONSTANTS can match one that differs in its partition equality predicates - and sizes are the similar-run check's job, not the key's. http://gerrit.cloudera.org:8080/#/c/24693/7/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/24693/7/fe/src/main/java/org/apache/impala/service/Frontend.java@2145 PS7, Line 2145: : explainStr); > This looks awkward.. To simplify the code, we can always show the HBO match Nice that you remembered the tool and had it in mind here :) That is worth measuring rather than guessing at, so I did. It never runs EXPLAIN itself: the plan reaches it as text someone pasted out of impala-shell, at whatever level they typed. So a query option and a level gate look the same from there. I ran its parser over a real EXTENDED plan with the annotation on every node. The details line costs it nothing - same cardinalities, same nodes. What does cost it is the "(from HBO)" suffix on the "tuple-ids=... cardinality=..." line, since it matches that line whole. That text is already in master, so it is a fix on my side either way, and a one-line one. My PS2 reason for the option doesn't hold any more now that the details are on their own line: test_hbo.py doesn't set explain_level, so hbo-*.test stays at STANDARD, and no planner test golden has an HBO match. So PS8 goes your way. One thing it changes - an EXPLAIN statement at the default level no longer carries the details in its profile, only ordinary queries do, since their plan renders at EXTENDED. That reads fine to me, but tell me if you'd rather keep the profile carrying it in both cases. http://gerrit.cloudera.org:8080/#/c/24693/7/tests/query_test/test_hbo.py File tests/query_test/test_hbo.py: http://gerrit.cloudera.org:8080/#/c/24693/7/tests/query_test/test_hbo.py@99 PS7, Line 99: 'explain_level': 2 > The query is not an EXPLAIN. I think we don't need to set explain_level her Dropped, here and in the aggressive case below. A non-EXPLAIN statement renders its plan at EXTENDED anyway, so the profile has the line without being asked. -- 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: 7 Gerrit-Owner: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aman Sinha <[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, 27 Aug 2026 09:50:43 +0000 Gerrit-HasComments: Yes
