adriangb opened a new issue, #25450: URL: https://github.com/apache/datafusion/issues/25450
### Is your feature request related to a problem or challenge? This is a trade-off item, not a bug report. `ExpressionPlacement` decides whether the leaf rules pull an expression out of a filter, a sort, a limit, an aggregate or a join, and push it down into the scan. The value comes from `ScalarUDFImpl::placement` in `datafusion/expr/src/udf.rs`, so it is a global constant of the function. Whether the push is a win depends on the data source. https://github.com/apache/datafusion/issues/25036 and https://github.com/apache/datafusion/pull/25025 make the case for `bit_length`, `octet_length` and `array_length`. On Vortex the win is large: the source answers `octet_length("URL")` from its layout and never loads the string bytes. ClickBench Q29 is the example in the issue: ```sql SELECT "CounterID", AVG(OCTET_LENGTH("URL")) AS l, COUNT(*) AS c FROM hits WHERE "URL" <> '' GROUP BY "CounterID" HAVING COUNT(*) > 100000 ORDER BY l DESC LIMIT 25; ``` On Parquet the same mark has a different effect. The push moves the function below every filter in the plan, so it runs on every row of the scan instead of on the rows that survive the filter. Parquet has to decode the string to measure it, so nothing is saved. The physical guard, `would_duplicate_costly_exprs` in `datafusion/datasource/src/file_scan_config/mod.rs`, only refuses the merge when an expression is referenced more than one time. A single reference merges into the scan. The same tension appears with `get_field`, which is already `MoveTowardsLeafNodes`. https://github.com/apache/datafusion/issues/14540 shows the plan it produces: `get_field` on both struct columns lands below the date filter. For a Parquet scan that absorbs the projection this is right, because the scan then reads one leaf instead of the whole struct. For a memory table, or for any source that cannot absorb the projection, the filter should run first. The review of PR 25025 also found a concrete gap inside the mark itself. `array_length(arr, dim)` with a non-literal `dim` does a per-row traversal that depends on `dim`, so it is not cheap. `get_field` already guards this case: its `placement` returns `KeepInPlace` unless every key argument is a literal (`datafusion/functions/src/core/getfield.rs`). `array_length` needs the same guard. The reviewer's test was `SELECT x, x AS x2 FROM (SELECT array_length(column1, column2) AS x FROM t)`, which merges into one `ProjectionExec` holding `array_length(column1, column2)` two times. ### Describe the solution you'd like Let the source say no. Add a hook that the leaf rules consult before they push an expression into a scan, in the same spirit as `TableProvider::supports_filters_pushdown`. A sketch: ```rust /// Which of `exprs` this source wants pushed into the scan. /// Default: accept every expression the planner offers. fn supports_leaf_expressions( &self, exprs: &[&Expr], ) -> Result<Vec<ExpressionPushdown>> { ... } ``` `ExpressionPushdown` needs at least two values, "absorb it" and "leave it above me". The hook belongs on `TableProvider`, and on `FileSource` for the file formats, so a Parquet source and a Vortex source can answer differently for `octet_length`. With the hook in place, `ScalarUDFImpl::placement` states what the expression *can* be, and the source states what it *should* be for that scan. No global constant has to be right for every source. ### Describe alternatives you've considered **A config option per function.** It does not help: one query can read a Parquet table and a Vortex table. **A cost model on `ExpressionPlacement`.** A number instead of four buckets. The number that decides this case is the cost of the function *at that source*, plus the selectivity of the filters it is pushed below. Neither is available at planning time. **Leave the placement global and rely on the physical guard.** `would_duplicate_costly_exprs` already blocks a merge when an expression is referenced more than one time. It does not block the single-reference case, which is the case in Q29, and it runs after the logical rules have already moved the expression below the filters. ### Additional context The hook has to avoid the problem that https://github.com/apache/datafusion/issues/19929 reports for `supports_filters_pushdown`. That method is called without the filters that a previous optimizer pass already pushed, so a provider whose answer depends on what it already holds gives a wrong answer on the second pass. A leaf expression hook is called from a rule that runs to a fixed point, so it will be called more than one time on the same plan. It must receive the expressions the scan already absorbed, and the answer must be stable when the rule asks again. Related: - https://github.com/apache/datafusion/pull/25025 - https://github.com/apache/datafusion/issues/25036 - https://github.com/apache/datafusion/issues/14540 - https://github.com/apache/datafusion/issues/19929 Tracked in the leaf-pushdown EPIC. -- 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]
