You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/09/18 02:11:37 UTC
[PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
adriangb opened a new pull request, #25456:
URL: https://github.com/apache/datafusion/pull/25456
## Which issue does this PR close?
- Closes https://github.com/apache/datafusion/issues/25329
## Rationale for this change
A function that keeps its place, for example a struct-returning UDF, is evaluated two times when the query hoists it into a common sub-expression and then reads one field of it inside a filter.
MRE:
```sql
CREATE TABLE t(a INT) AS VALUES (1), (2);
EXPLAIN SELECT a FROM t WHERE CASE WHEN arrow_field(a) IS NOT NULL THEN arrow_field(a)['nullable'] IS NULL END;
```
Before, on `main`. `arrow_field(a)` is in the plan two times, so it runs two times for each row.
```text
logical_plan
01)Projection: t.a
02)--Filter: CASE WHEN __common_expr_7 IS NOT NULL THEN __datafusion_extracted_8 IS NULL END
03)----Projection: arrow_field(t.a) AS __common_expr_7, t.a, get_field(arrow_field(t.a), Utf8("nullable")) AS __datafusion_extracted_8
04)------TableScan: t projection=[a]
physical_plan
01)FilterExec: CASE WHEN __common_expr_7@0 IS NOT NULL THEN __datafusion_extracted_8@2 IS NULL END, projection=[a@1]
02)--ProjectionExec: expr=[arrow_field(a@0) as __common_expr_7, a@0 as a, get_field(arrow_field(a@0), nullable) as __datafusion_extracted_8]
03)----DataSourceExec: partitions=1, partition_sizes=[1]
```
After, with this PR. `arrow_field(a)` is in the plan one time:
```text
logical_plan
01)Projection: t.a
02)--Filter: CASE WHEN __common_expr_1 IS NOT NULL THEN get_field(__common_expr_1, Utf8("nullable")) IS NULL END
03)----Projection: arrow_field(t.a) AS __common_expr_1, t.a
04)------TableScan: t projection=[a]
physical_plan
01)FilterExec: CASE WHEN __common_expr_1@0 IS NOT NULL THEN get_field(__common_expr_1@0, nullable) IS NULL END, projection=[a@1]
02)--ProjectionExec: expr=[arrow_field(a@0) as __common_expr_1, a@0 as a]
03)----DataSourceExec: partitions=1, partition_sizes=[1]
```
The same statement in a `SELECT` list runs the function one time since https://github.com/apache/datafusion/pull/23691. That fix counts the reference sites inside one projection. In the filter shape the two sites are in two different nodes, the filter and the extracted expression, so the count is 1 and the guard does not fire.
`CommonSubexprEliminate` puts an expression into its own column so that the plan evaluates it one time. Four rules then replace a column reference with the expression that defines it. Each rule got its own guard against the duplication that this causes, one guard for each bug report:
| Guard | Rule | Added by |
| --- | --- | --- |
| `would_duplicate_volatile` (3 call sites) | `ExtractLeafExpressions`, `PushDownLeafProjections` | https://github.com/apache/datafusion/pull/24720 |
| `merge_would_duplicate_kept_expr` | `PushDownLeafProjections` | https://github.com/apache/datafusion/pull/23691 |
| the volatile and placement partition in `rewrite_projection` | `PushDownFilter` | - |
| the referral count in `merge_consecutive_projections_one_level` | `OptimizeProjections` | https://github.com/apache/datafusion/issues/8296 |
Each guard sees only its own rule and its own node. This PR puts the decision in one place, so a rule can not miss a reference site again.
## What changes are included in this PR?
A crate-private `ProjectionInliner` (`datafusion/optimizer/src/projection_inliner.rs`) with one policy:
1. Classify each definition of the projection by the cost of one more evaluation: free (a column, a literal or a `MoveTowardsLeafNodes` expression), cheap (arithmetic, `CAST`, `CASE` and similar kernels), expensive (contains a scalar function that reports `KeepInPlace`), or forbidden (volatile, or contains a subquery).
2. Count the evaluations that the plan has after the rewrite. This counts the references in the expressions that the rule inlines, and the columns that the other consumers keep.
3. Report the definitions that must stay where they are: a forbidden definition, and a definition above the cost that the caller accepts that the plan would evaluate more than one time.
The four rules call it instead of building their own replacement map:
- `OptimizeProjections` accepts free definitions only, which is the behaviour of https://github.com/apache/datafusion/issues/8296. The plans do not change.
- `PushDownFilter` accepts cheap definitions. A predicate stays above the projection when it references a volatile definition, a `MoveTowardsLeafNodes` definition (as before), or an expensive definition that the predicates reference more than one time. A predicate with one reference is still pushed, so a cheap cast or an arithmetic expression still reaches the scan.
- `ExtractLeafExpressions` and `PushDownLeafProjections` accept free definitions only, and count the extracted expressions together with the columns that the nodes above reference directly. This is the fix for the issue.
Deleted: `would_duplicate_volatile`, `volatile_output_columns`, `merge_would_duplicate_kept_expr`, `build_projection_replace_map` and the `contain` helper of `push_down_filter`.
The commits are one for the new module, one for each rule, and one for the tests.
This PR is complementary to the draft https://github.com/apache/datafusion/pull/25388. That draft stops a push-down whenever the projection expression is `KeepInPlace`, which is a wider rule: its own snapshots show that a cast and an arithmetic expression no longer reach `partial_filters` and `full_filters`. This PR only stops a rewrite that evaluates a definition more times than before, which is a bug in every case. cc @pepijnve
## What is the testing strategy for this PR?
- `datafusion/sqllogictest/test_files/cse.slt`: both end-to-end shapes, the projection shape of https://github.com/apache/datafusion/issues/23655 and the filter shape of this issue. Both plans must show `arrow_field(a)` one time.
- `projection_inliner.rs`: unit tests for the cost classification, the evaluation count, the substitution and the volatile case.
- `push_down_filter.rs`: `filter_with_repeated_expensive_reference_not_pushed_through_projection`, `filter_with_single_expensive_reference_pushed_through_projection` and `filter_not_pushed_through_nested_computed_projection` (the shape of the draft above, with the plan that the narrow policy gives).
- `extract_leaf_expressions.rs`: `test_struct_returning_udf_in_filter_evaluated_once`.
No existing snapshot and no existing `.slt` expectation changed.
## Are there any user-facing changes?
Plans that evaluated a `KeepInPlace` expression two times now evaluate it one time. There is no API change: `ProjectionInliner` is crate-private.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#discussion_r4045318003
##########
datafusion/optimizer/src/projection_inliner.rs:
##########
@@ -0,0 +1,497 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! A single, cost-aware check for rewriting expressions through a projection.
+//!
+//! Many rules move an expression that is written over the output of a
+//! projection below that projection, or merge it into the projection. To do
+//! this, they replace each column reference with the expression that defines
+//! the column. This is called *inlining*.
+//!
+//! Inlining can make the plan evaluate a definition more times than before:
+//!
+//! - The consumer can reference the column more than one time.
+//! - The projection can stay in the plan and still compute the definition.
+//!
+//! When the definition is volatile, this changes the query result. When the
+//! definition is expensive, this makes the query slower. A typical example is a
+//! column that [`CommonSubexprEliminate`] created on purpose, so that an
+//! expensive expression is computed one time only.
+//!
+//! [`ProjectionInliner`] is the one place that makes this decision. Rules call
+//! it instead of doing the substitution themselves.
+//!
+//! [`CommonSubexprEliminate`]: crate::common_subexpr_eliminate::CommonSubexprEliminate
+
+use std::collections::{HashMap, HashSet};
+
+use datafusion_common::tree_node::{
+ Transformed, TransformedResult, TreeNode, TreeNodeRecursion,
+};
+use datafusion_common::{Column, Result};
+use datafusion_expr::{Expr, ExpressionPlacement, Projection};
+
+/// The cost of one more evaluation of an expression.
+///
+/// The variants are in increasing order of cost.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum DuplicationCost {
Review Comment:
This enumeration seems to overlap largely with the existing expression placement enumeration. Are those two trying to express the same thing or is there a fundamental semantic difference?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#issuecomment-6406198695
Marked as draft after 14 days without activity: merge conflicts.
Please address these items and mark **Ready for review** when ready.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#issuecomment-5727382333
I haven't gone through the code changes in detail yet, but the description sounds like a good way forward. Implementing the assessment once rather than trying to patch things ad hoc per optimiser is the way to go.
An open question I still had related to keep-in-place was if this should be refined a bit. On #25388 I tweaked `Expr::placement` so that for simple expressions (unary and binary ops mostly) it is based on the operands placement rather than being keep-in-place. Is that something we would want to take along here? Or should I make a separate PR out of that?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#discussion_r4419702125
##########
datafusion/optimizer/src/projection_inliner.rs:
##########
@@ -0,0 +1,540 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: See the NOTICE file distributed with this work for additional information regarding copyright ownership
+// SPDX-FileContributor: Licensed to the Apache Software Foundation (ASF) under one or more contributor license agreements
+
+//! A single, cost-aware check for rewriting expressions through a projection.
+//!
+//! Many rules move an expression that is written over the output of a
+//! projection below that projection, or merge it into the projection. To do
+//! this, they replace each column reference with the expression that defines
+//! the column. This is called *inlining*.
+//!
+//! Inlining can make the plan evaluate a definition more times than before:
+//!
+//! - The consumer can reference the column more than one time.
+//! - The projection can stay in the plan and still compute the definition.
+//!
+//! When the definition is volatile, this changes the query result. When the
+//! definition is expensive, this makes the query slower. A typical example is a
+//! column that [`CommonSubexprEliminate`] created on purpose, so that an
+//! expensive expression is computed one time only.
+//!
+//! [`ProjectionInliner`] is the one place that makes this decision. Rules call
+//! it instead of doing the substitution themselves.
+//!
+//! [`CommonSubexprEliminate`]: crate::common_subexpr_eliminate::CommonSubexprEliminate
+
+use std::collections::{HashMap, HashSet};
+
+use datafusion_common::tree_node::{
+ Transformed, TransformedResult, TreeNode, TreeNodeRecursion,
+};
+use datafusion_common::{Column, Result};
+use datafusion_expr::{Expr, ExpressionPlacement, Projection};
+
+/// The cost of one more evaluation of an expression.
+///
+/// The variants are in increasing order of cost.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum DuplicationCost {
+ /// Column references and expressions that report
+ /// [`ExpressionPlacement::MoveTowardsLeafNodes`] (for example struct field
+ /// access). These are free, or almost free, to evaluate again.
+ Free,
+ /// Built-in operators over free or cheap operands: arithmetic,
+ /// comparisons, `CAST`, `CASE`, `IS NULL`, and similar. They are
+ /// vectorized kernels, so an extra evaluation costs little.
+ ///
+ /// A bare literal is also in this class. This keeps the merge guard of
+ /// `OptimizeProjections` (issue #8296) unchanged.
+ Cheap,
+ /// Contains a scalar function that reports
+ /// [`ExpressionPlacement::KeepInPlace`] for column arguments, or a
+ /// higher-order function. This is the default for a user-defined
+ /// function, so the optimizer must assume that it can be arbitrarily
+ /// expensive.
+ Expensive,
Review Comment:
This is again creating a mess of `interpret KeepInPlace as expensive` which is not clear we want to do, as already pointed out in https://github.com/apache/datafusion/pull/25456/changes#r4045318003
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#issuecomment-5724325157
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25456?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.35075%` with `41 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.34%. Comparing base ([`3a647e4`](https://app.codecov.io/gh/apache/datafusion/commit/3a647e49dd79b19d92eddd094bf927f28363332d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`91a5018`](https://app.codecov.io/gh/apache/datafusion/commit/91a50187982e8344e43765eae799ba59d2e16f66?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25456?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...tafusion/optimizer/src/extract\_leaf\_expressions.rs](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fextract_leaf_expressions.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL2V4dHJhY3RfbGVhZl9leHByZXNzaW9ucy5ycw==) | 88.26% | [11 Missing and 12 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/optimizer/src/push\_down\_filter.rs](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fpush_down_filter.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3B1c2hfZG93bl9maWx0ZXIucnM=) | 78.08% | [2 Missing and 14 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/optimizer/src/projection\_inliner.rs](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&filepath=datafusion%2Foptimizer%2Fsrc%2Fprojection_inliner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9vcHRpbWl6ZXIvc3JjL3Byb2plY3Rpb25faW5saW5lci5ycw==) | 99.23% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25456?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25456 +/- ##
========================================
Coverage 82.33% 82.34%
========================================
Files 1137 1138 +1
Lines 432498 432839 +341
Branches 432498 432839 +341
========================================
+ Hits 356115 356418 +303
- Misses 54844 54864 +20
- Partials 21539 21557 +18
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25456?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: one inlining policy for the rules that inline a projection column [datafusion]
Posted by "pepijnve (via GitHub)" <gi...@apache.org>.
pepijnve commented on code in PR #25456:
URL: https://github.com/apache/datafusion/pull/25456#discussion_r4045318003
##########
datafusion/optimizer/src/projection_inliner.rs:
##########
@@ -0,0 +1,497 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! A single, cost-aware check for rewriting expressions through a projection.
+//!
+//! Many rules move an expression that is written over the output of a
+//! projection below that projection, or merge it into the projection. To do
+//! this, they replace each column reference with the expression that defines
+//! the column. This is called *inlining*.
+//!
+//! Inlining can make the plan evaluate a definition more times than before:
+//!
+//! - The consumer can reference the column more than one time.
+//! - The projection can stay in the plan and still compute the definition.
+//!
+//! When the definition is volatile, this changes the query result. When the
+//! definition is expensive, this makes the query slower. A typical example is a
+//! column that [`CommonSubexprEliminate`] created on purpose, so that an
+//! expensive expression is computed one time only.
+//!
+//! [`ProjectionInliner`] is the one place that makes this decision. Rules call
+//! it instead of doing the substitution themselves.
+//!
+//! [`CommonSubexprEliminate`]: crate::common_subexpr_eliminate::CommonSubexprEliminate
+
+use std::collections::{HashMap, HashSet};
+
+use datafusion_common::tree_node::{
+ Transformed, TransformedResult, TreeNode, TreeNodeRecursion,
+};
+use datafusion_common::{Column, Result};
+use datafusion_expr::{Expr, ExpressionPlacement, Projection};
+
+/// The cost of one more evaluation of an expression.
+///
+/// The variants are in increasing order of cost.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) enum DuplicationCost {
Review Comment:
This enumeration and the way `DuplicationCost::of` calculates it seems to overlap largely with the existing expression placement enumeration and `Expr::placement`. Are those two trying to express the same thing or is there a fundamental semantic difference?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org