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: (3 comments) 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()); > Using `relNode` fixes the lookup. One issue with the regression test: `test It's actually a little harder to make this test fail than it seems. In order for this test to fail, we need to get to this point recursively as you initially mentioned. The first step in the recursion is to call "getJoinNullPercentageEstimate". This handles most join estimates. The only places it passes through to this point is if that method returns null. It only returns null if a) the column is not an input ref or b) an anti-join or semi-join. Then the problem is: For a) it still wouldn't hit this code because of line 199. For b) we can't generate an anti-join yet. And a semi-join only selects columns from the left side. 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(); > The guard avoids the exception, but this test uses `b.id IS NOT NULL`. The I'm not sure I'm totally getting what you're saying here... The "is not null" has a wrapper call to "is null" at the top level, so this shouldn't matter. Having said this though, I don't have an idea on what to do here. If it hits this case, we know where on the "outer" side, so nulls will get generated (which is perhaps what you were saying?) But estimating how many nulls involves knowing which rows didn't match which I'm not sure how to estimate. So I decided to just use the DEFAULT value here rather than returning to the caller which might come up with a non-default, more prone to error estimation. 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); > The one-to-one cases work now, but the one-to-many case is still off. For ` Same problem as the non-equality. I don't know how to determine the one-to-many case based on just the statistics. I'm not sure how to do that with what you are saying? Do you have a formula in mind to handle this? If we can't come up with a formula, maybe just assuming one-to-one is the best we can do? It's prolly better than just defaulting to DEFAULT, but that's just my intution, and I can be convinced otherwise. -- 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 15:39:19 +0000 Gerrit-HasComments: Yes
