Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24297 )
Change subject: IMPALA-14600: Support HBO for AggregationNode cardinality ...................................................................... Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/24297/8/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/24297/8/common/thrift/Frontend.thrift@789 PS8, Line 789: // HDFS URI for the jar > I'm also gonna work under the assumption that we can create runtime filters > in the thrift structure that aren't "effective"? Not sure if that is true or > what that means, but I'm gonna assume that's the case. Yeah, in IMPALA-14796 we distinguish runtime filters as effective vs. ineffective by whether they actually filter out any data on the target nodes. Note that a runtime filter can be assigned to multiple target nodes (i.e. scan nodes on different tables). So it could be effective on some nodes but ineffective on other nodes. > Can each TPlanNode just contain all runtime filter Ids that may be affected? > ... > So now if we have the TPlanNode contain a list of runtime filter Ids that > could affect it, I think the backend code doesn't change too much? I think it's not enough since each runtime filter has multiple target nodes. What we need is a list of (filter_id, target_node_id) pairs instead of a filter_id list. And for each (filter_id, target_node_id) pair, it needs to be set from the target scan node till the source join node (exclusive). I.e., for a runtime filter built at join S and applied at target scan T, the pair propagates up every node on the path T -> S (exclusive of S). With this, we can simplify the BE code a lot - for each node, check if any pairs appear in the effective (filter_id, target_node_id) pairs (known after execution). The downside is the pairs are duplicated through the plan tree. For a left-deep plan, each join node produces runtime filters and assigns them to the left side. We end up have lots of duplicated (filter_id, target_node_id) pairs. I tend to generate these lists where they are used, i.e. in BE. > This structure seems a bit off to me because this is information that can be > placed in the TPlanNode which can be made aware of its parentId? That's a good idea. Moving these parent ids into each TPlanNode seems cleaner. Done in the new patch set. http://gerrit.cloudera.org:8080/#/c/24297/8/fe/src/main/java/org/apache/impala/analysis/SlotRef.java File fe/src/main/java/org/apache/impala/analysis/SlotRef.java: http://gerrit.cloudera.org:8080/#/c/24297/8/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@341 PS8, Line 341: Preconditions.checkState(false, "Unexpected use of old toThrift() signature"); > nit: StringUtils.isEmpty(name) Done http://gerrit.cloudera.org:8080/#/c/24297/8/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@345 PS8, Line 345: protected void toThrift(TExprNode msg, ThriftSerializationCtx serialCtx) { > Ooh, weird situation: Is it even possible to have a naked backquote at the That's a good point. The current code is unreliable. Changed this to use the source expr. http://gerrit.cloudera.org:8080/#/c/24297/8/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java: http://gerrit.cloudera.org:8080/#/c/24297/8/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java@118 PS8, Line 118: > Just want to make sure here... That's a good point! It does affect the cardinality of ancestor nodes if the output results are used in filters, group-by, join keys. Removed this. We can leave it as a future improvement to identify these cases and only skip AnalyticEvalNode if it doesn't matter. http://gerrit.cloudera.org:8080/#/c/24297/8/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/24297/8/fe/src/main/java/org/apache/impala/service/Frontend.java@2454 PS8, Line 2454: TPlannerType fallbackPlanner = queryOptions.getFallback_planner(); > nit: prolly either both buildNodeParentIds should be static or neither one? Removed these in the new patch set. http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java File fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java: http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@257 PS9, Line 257: * Test for the FOR_HBO rendering of a grouping SlotRef on a column whose name contains > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@259 PS9, Line 259: */ > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@261 PS9, Line 261: public void testAggKeyWithSpecialColumnName() { > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@275 PS9, Line 275: assertEquals("`name`one`", substituted.toSql(ToSqlOptions.FOR_HBO)); > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@282 PS9, Line 282: > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/24297/9/fe/src/test/java/org/apache/impala/planner/HboKeyStringTest.java@285 PS9, Line 285: > line too long (91 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/24297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0fafaf9d827f3bf533b1af7e62fdb2303c126ce Gerrit-Change-Number: 24297 Gerrit-PatchSet: 10 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 06 Jul 2026 06:39:46 +0000 Gerrit-HasComments: Yes
