Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24605 )
Change subject: IMPALA-15195: Calcite Planner performance improvements for tpcds ...................................................................... Patch Set 9: (4 comments) PS9 covers the things I flagged: the childRel_/relNode test reproduces now, and the unmatched-rows formula is what I had in mind. Two things left inline - FULL joins take the wrong side of that formula, and there is a reachable 0/0 - plus two nits. Both are estimate quality rather than correctness, so neither is blocking. http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java: http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@206 PS9, Line 206: RelColumnOrigin originCol = mq_.getColumnOrigin(relNode, inputRef.getIndex()); nit: RelColumnOrigin.isDerived() is not checked, so a derived column picks up the base column's null stats - sum(x) IS NULL over an aggregate would use x's numNulls. Falling back to DEFAULT_IS_NULL_PERCENTAGE when isDerived() is true would be closer. http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@287 PS9, Line 287: Math.min(info.getUnmatchedRowsToOuterJoin()/outerRowCount, 1.0); FULL joins reach this from both sides - the early return at 256-258 only covers LEFT and RIGHT. So for a FULL join both a left-side and a right-side column get getUnmatchedRowsToOuterJoin(), which returns unmatchedLeft + unmatchedRight. Only the opposite side generates nulls for a given column: unmatchedRight for a left-side column, unmatchedLeft for a right-side one. That gap is in the formula I gave you - I wrote that FULL "includes both", but I meant the output row count, not a single column's null rate. Concretely, left 100 rows / NDV 100 against right 10 rows / NDV 10 gives unmatchedLeft = 90, unmatchedRight = 0 and outerRowCount = 100, so a left-side column comes out at 90% null instead of ~0%. Passing the side into getUnmatchedRowsToOuterJoin(), or splitting it in two, would cover it. LEFT and RIGHT are already right because the non-outer side returns early. http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/JoinRelationInfo.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/JoinRelationInfo.java: http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/JoinRelationInfo.java@251 PS9, Line 251: unmatchedRows += leftRows * (1 - overlappedNdvs / leftNdvs); 0/0 -> NaN when the outer side's NDV is 0. getDistinctRowCount(TableScan) in ImpalaRelMdDistinctRowCount returns the raw catalog stat, and 0 is a real value there for an all-null column - ExprCardinalityTest.java:151 pins NDV(functional.nullrows.null_str) at 0, with 26 nulls out of 26 rows. Then leftNdvs = 0, overlappedNdvs = min(0, rightNdvs) = 0, and leftRows * (1 - 0/0) is NaN. Math.min and Math.max at FilterSelectivityEstimator.java:287-288 both propagate NaN, so it lands in the selectivity. Query shape that gets there: SELECT b.string_col FROM functional.nullrows a LEFT JOIN functional.alltypestiny b ON a.null_str = b.string_col WHERE b.string_col IS NULL The zero-row guard at FilterSelectivityEstimator.java:283 does not help, since outerRowCount is non-zero. NDV = 0 means nothing on the other side can match, so the answer is 100% unmatched - guarding leftNdvs/rightNdvs <= 0 would improve the estimate, not just avoid the NaN. Separately, 237-238 unbox lhsNdv_/rhsNdv_ while createEqualityConjunction() only null-checks the row counts. mq.getDistinctRowCount() is @Nullable and Calcite's default impl does return null for some rel types. getDistinctRows() at 188/191 has the same exposure, so this predates the patch - just noting the new code inherits it. http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TestCalciteStats.java File java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TestCalciteStats.java: http://gerrit.cloudera.org:8080/#/c/24605/9/java/calcite-planner/src/test/java/org/apache/impala/calcite/planner/TestCalciteStats.java@551 PS9, Line 551: Double dd = mq.getRowCount(logicalPlan); nit: `dd` is unused - same at 566 and 584. -- To view, visit http://gerrit.cloudera.org:8080/24605 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ee6662dc6500ea6ebd9f859de4df1740c51a058 Gerrit-Change-Number: 24605 Gerrit-PatchSet: 9 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 07 Aug 2026 07:56:25 +0000 Gerrit-HasComments: Yes
