Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24556 )
Change subject: IMPALA-15127: Support HBO for UnionNode cardinality ...................................................................... Patch Set 10: (5 comments) Thanks for the comments! They are quite helpful! http://gerrit.cloudera.org:8080/#/c/24556/8/fe/src/main/java/org/apache/impala/planner/UnionNode.java File fe/src/main/java/org/apache/impala/planner/UnionNode.java: http://gerrit.cloudera.org:8080/#/c/24556/8/fe/src/main/java/org/apache/impala/planner/UnionNode.java@376 PS8, Line 376: StringBuilder sb = new StringBuilder(statsType.name()).append(":UnionNode:"); > But I think I disagree with that. I think it's still better than nothing. E.g. the following two queries only differ in the number of constant union operands. The JoinNode cardinalities differ a lot. Using different HBO keys for the UnionNode helps to have different keys for the JoinNode. select count(*) from functional.alltypes a inner join ( select distinct int_col from functional.alltypestiny union all values (2), (3), (4) ) b on a.int_col = b.int_col; select count(*) from functional.alltypes a inner join ( select distinct int_col from functional.alltypestiny union all values (2), (3) ) b on a.int_col = b.int_col; http://gerrit.cloudera.org:8080/#/c/24556/8/fe/src/main/java/org/apache/impala/planner/UnionNode.java@383 PS8, Line 383: if (i > 0) sb.append(","); > Should we include `limit_` in this key, as `HdfsScanNode` does? A top-level Nice catch! LIMIT is also missing in AggregationNode. http://gerrit.cloudera.org:8080/#/c/24556/8/fe/src/main/java/org/apache/impala/planner/UnionNode.java@391 PS8, Line 391: public void appendScanInputStats(TPlanNodeRun execStats) { > I don't think sorting this list by `EXPR_REWRITE` keeps it aligned with eve That's a good example! Initially I thought the IGNORE_PARTITION_CONSTANTS strategy assumes that partitions in the same table have similar stats. So it's OK to let these two queries match. But if the partitions have different numbers of input rows and if int_col=1 and int_col=-999 have different selectivity, matching these two queries gives a wrong cardinality. BTW, I think the planner should merge the union operands into one if they are simple scan on the same table, though we don't have such optimization yet. I'll think more about this and address this in the next patch set. http://gerrit.cloudera.org:8080/#/c/24556/8/fe/src/main/java/org/apache/impala/planner/UnionNode.java@415 PS8, Line 415: msg.union_node = new TUnionNode( > `populateHboThriftFields()` also runs for a constants-only Union. Its key i Nice catch! We shouldn't write HBO stats for such UnionNodes. http://gerrit.cloudera.org:8080/#/c/24556/8/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/24556/8/fe/src/main/java/org/apache/impala/service/HistoricalStats.java@118 PS8, Line 118: for (int i = 0; i < runs.size(); i++) { > Similarity ranges can overlap, so returning the first match does not always I think this is the limitaion of simple threshold matching. For this example, keeping 120 can help more lookups, e.g. for 130, so it's still useful. The run of 120 might not be a stale stats. It could belong to another kind of partitions. So I think keeping it still helps. -- To view, visit http://gerrit.cloudera.org:8080/24556 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie228f530bdcb171d3b717966673164bf9a4c45c8 Gerrit-Change-Number: 24556 Gerrit-PatchSet: 10 Gerrit-Owner: Quanlong Huang <[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: Wed, 05 Aug 2026 12:57:20 +0000 Gerrit-HasComments: Yes
