peter-toth commented on PR #58145: URL: https://github.com/apache/spark/pull/58145#issuecomment-6542082777
@aokolnychyi @szehon-ho, you're right that the `min` loses the link between a source predicate and what was inferred from it. It was my suggestion in [r3922411356](https://github.com/apache/spark/pull/58145#discussion_r3922411356), and it assumed a single query predicate. With an unrelated, estimable predicate next to it, the result can be too high. The inferred group is estimated alone and misses that predicate. Before choosing between planting all inferred predicates as `Filter`s and a mapping, a few thoughts: 1. **Why do we need the `reflectsFullyPushedDownFilters() == false` path at all?** If a source can use an inferred predicate for pruning, why can't it adjust its statistics as well, e.g. from the record counts of the files it kept? With the default `true`, Spark would then need no inferred filters, no re-added filters and no extra estimation logic. 2. **A mapping is not very robust either.** The inferred predicates would stay metadata, which is good. But the visitor has to find each source predicate in the final plan: - A fully pushed source predicate is in `pushedFilters`, so that one is stable. - A residual source predicate stays in a `Filter`, and the optimizer rules after pushdown can rewrite it or merge it with other predicates. Then the key is not found, and the estimate silently falls back. This is the same kind of exact-expression matching that cloud-fan called fragile in [r3904675085](https://github.com/apache/spark/pull/58145#discussion_r3904675085), although a miss here only affects the estimate. - One inferred predicate can come from several conjuncts, e.g. `ts >= X AND ts < Y` gives one `date` range. So a key would be a set of predicates, not one expression. 3. **Planting them as `Filter`s re-adds filters that execution doesn't need.** - The inferred predicates become executable `Filter`s. With `false`, so do the fully pushed ones (SPARK-58315). - They run again on every row the scan returns, although none of them can drop a row. - Planting them doesn't depend on CBO. Without CBO the `Filter` estimate ignores selectivity, so these filters run for nothing. - Double counting errs in the risky direction. If the source predicate is estimable, its selectivity is counted twice. The row count then comes out too low, which can push a large side below the broadcast threshold. - In the measurement at [r3922411356](https://github.com/apache/spark/pull/58145#discussion_r3922411356), the estimate was 11 rows with the inferred filter and 101 without it. The real count was 100. - The `min` can only err towards too many rows. On its own, the inferred group's estimate is an upper bound for the real count. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
