Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24605 )
Change subject: IMPALA-15195: Calcite Planner performance improvements for tpcds ...................................................................... Patch Set 6: (8 comments) Sigh, I got too focused on getting the performance right that I neglected the unit tests for the "isNull" changes. Thanks for catching that. The unit tests are now added and the numbers and cases mentioned should be checked. http://gerrit.cloudera.org:8080/#/c/24605/6/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/24605/6/fe/src/main/java/org/apache/impala/planner/Planner.java@1022 PS6, Line 1022: avoid all invert joins in its : // optimization phase, > nit: This is still a bit confusing .. it is saying 'avoid all invert joins' Hopefully the comment is better now. http://gerrit.cloudera.org:8080/#/c/24605/6/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/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@205 PS6, Line 205: RelColumnOrigin originCol = mq_.getColumnOrigin(childRel_, inputRef.getIndex()); > This needs to use relNode, not childRel_. After recursing into the right si Added some right join tests. http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@212 PS6, Line 212: return getNumNulls(columnNum, table) / table.getRowCount(); > This can return NaN for an empty table: getNumNulls() is 0 and table.getRow Protecting the denominator now http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@212 PS6, Line 212: return getNumNulls(columnNum, table) / table.getRowCount(); > nit: this assumes that one of the operands of the division is a double. Be Done http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@266 PS6, Line 266: The hope > nit: this is the exact behavior, so this phrase can be skipped. Done http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@274 PS6, Line 274: Double innerRowCount = info.getRowCount(); > This throws for a valid non-equi outer join. With l LEFT JOIN r ON l.x < r. testLeftJoinWithNonEqualityPredicate tests this http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@282 PS6, Line 282: Double percentage = Math.min(innerRowCount/nonOuterJoinRowCount, 1.0); > This is the matched-row ratio, not the null percentage. With a LEFT JOIN an Yeah, this math was way off. Unit tests should now work for this. http://gerrit.cloudera.org:8080/#/c/24605/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/FilterSelectivityEstimator.java@288 PS6, Line 288: // outer join > We cannot pass the same ordinal through an arbitrary Project. A projection The testLeftJoinWithIsNullTestFilterAndProject test should cover this. -- 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: 6 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: Thu, 06 Aug 2026 04:34:48 +0000 Gerrit-HasComments: Yes
