You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "kumarUjjawal (via GitHub)" <gi...@apache.org> on 2026/06/03 16:36:46 UTC
[PR] Fix volatile scalar subquery deduplication [datafusion]
kumarUjjawal opened a new pull request, #22736:
URL: https://github.com/apache/datafusion/pull/22736
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #21619
## Rationale for this change
Identical uncorrelated scalar subqueries are deduplicated, but this is not correct when the subquery contains volatile expressions such as `random()`.
For example, `(SELECT random()) - (SELECT random())` must evaluate both scalar subquery occurrences independently.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
- Detect volatility inside subquery plans.
- Avoid CSE deduplication for volatile scalar subqueries.
- Keep volatile scalar subquery occurrences distinct in physical planning.
- Preserve deduplication for non-volatile scalar subqueries.
- Add regression coverage for same-node, cross-node, nested, and shared-expression cases.
- Update upgrade notes for the public `ExecutionProps` field type.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Yes. Queries with repeated volatile scalar subqueries now evaluate each occurrence independently instead of incorrectly reusing one result.
There is also a public API type change for `ExecutionProps::subquery_indexes`, documented in the upgrade notes.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#issuecomment-4619551122
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v53.1.0 (current)
Built [ 75.963s] (current)
Parsing datafusion v53.1.0 (current)
Parsed [ 0.029s] (current)
Building datafusion v53.1.0 (baseline)
Built [ 75.587s] (baseline)
Parsing datafusion v53.1.0 (baseline)
Parsed [ 0.029s] (baseline)
Checking datafusion v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.700s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 154.166s] datafusion
Building datafusion-expr v53.1.0 (current)
Built [ 20.327s] (current)
Parsing datafusion-expr v53.1.0 (current)
Parsed [ 0.061s] (current)
Building datafusion-expr v53.1.0 (baseline)
Built [ 20.264s] (baseline)
Parsing datafusion-expr v53.1.0 (baseline)
Parsed [ 0.062s] (baseline)
Checking datafusion-expr v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 1.422s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field Subquery.scalar_subquery_index in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/plan.rs:4359
field Subquery.scalar_subquery_index in /home/runner/work/datafusion/datafusion/datafusion/expr/src/logical_plan/plan.rs:4359
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 43.420s] datafusion-expr
Building datafusion-optimizer v53.1.0 (current)
Built [ 20.279s] (current)
Parsing datafusion-optimizer v53.1.0 (current)
Parsed [ 0.025s] (current)
Building datafusion-optimizer v53.1.0 (baseline)
Built [ 20.376s] (baseline)
Parsing datafusion-optimizer v53.1.0 (baseline)
Parsed [ 0.026s] (baseline)
Checking datafusion-optimizer v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.189s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 43.333s] datafusion-optimizer
Building datafusion-physical-expr v53.1.0 (current)
Built [ 22.064s] (current)
Parsing datafusion-physical-expr v53.1.0 (current)
Parsed [ 0.040s] (current)
Building datafusion-physical-expr v53.1.0 (baseline)
Built [ 21.911s] (baseline)
Parsing datafusion-physical-expr v53.1.0 (baseline)
Parsed [ 0.041s] (baseline)
Checking datafusion-physical-expr v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.379s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 45.630s] datafusion-physical-expr
Building datafusion-proto v53.1.0 (current)
Built [ 44.339s] (current)
Parsing datafusion-proto v53.1.0 (current)
Parsed [ 0.015s] (current)
Building datafusion-proto v53.1.0 (baseline)
Built [ 44.196s] (baseline)
Parsing datafusion-proto v53.1.0 (baseline)
Parsed [ 0.016s] (baseline)
Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.311s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 90.832s] datafusion-proto
Building datafusion-sql v53.1.0 (current)
Built [ 31.532s] (current)
Parsing datafusion-sql v53.1.0 (current)
Parsed [ 0.025s] (current)
Building datafusion-sql v53.1.0 (baseline)
Built [ 31.454s] (baseline)
Parsing datafusion-sql v53.1.0 (baseline)
Parsed [ 0.027s] (baseline)
Checking datafusion-sql v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.263s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 64.809s] datafusion-sql
Building datafusion-sqllogictest v53.1.0 (current)
Built [ 129.162s] (current)
Parsing datafusion-sqllogictest v53.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-sqllogictest v53.1.0 (baseline)
Built [ 128.983s] (baseline)
Parsing datafusion-sqllogictest v53.1.0 (baseline)
Parsed [ 0.021s] (baseline)
Checking datafusion-sqllogictest v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.107s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 260.778s] datafusion-sqllogictest
Building datafusion-substrait v53.1.0 (current)
Built [ 260.385s] (current)
Parsing datafusion-substrait v53.1.0 (current)
Parsed [ 0.014s] (current)
Building datafusion-substrait v53.1.0 (baseline)
Built [ 267.532s] (baseline)
Parsing datafusion-substrait v53.1.0 (baseline)
Parsed [ 0.015s] (baseline)
Checking datafusion-substrait v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.233s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 530.682s] datafusion-substrait
```
</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 volatile scalar subquery deduplication [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#issuecomment-5186755936
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#issuecomment-4614660891
@neilconway would you be available to review this pr?
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #22736: Fix volatile scalar subquery deduplication
URL: https://github.com/apache/datafusion/pull/22736
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "nathanb9 (via GitHub)" <gi...@apache.org>.
nathanb9 commented on code in PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#discussion_r3356010325
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4387,8 +4407,49 @@ impl Subquery {
subquery: plan,
outer_ref_columns: self.outer_ref_columns.clone(),
spans: Spans::new(),
+ scalar_subquery_index: self.scalar_subquery_index,
+ }
+ }
+
+ pub fn with_scalar_subquery_index(&self, index: SubqueryIndex) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: Some(index),
+ }
+ }
+
+ pub fn without_scalar_subquery_index(&self) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: None,
}
}
+
+ /// Returns `true` if this subquery's plan contains a volatile expression.
+ pub fn is_volatile(&self) -> bool {
+ plan_contains_volatile(&self.subquery)
+ }
+}
+
+/// Returns `true` if any expression in `plan` is volatile.
+fn plan_contains_volatile(plan: &LogicalPlan) -> bool {
Review Comment:
small but is it possible to make this public?
Would be useful for detecting volatile functions when materializing subplans (https://github.com/apache/datafusion/issues/22676)
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#discussion_r3351560421
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -442,15 +474,30 @@ impl DefaultPhysicalPlanner {
// scalar subqueries to joins, so none should reach this point.
// Skip collection in that case to avoid creating a no-op
// `ScalarSubqueryExec` wrapper.
- let all_subqueries = if session_state
+ if !session_state
.config_options()
.optimizer
.enable_physical_uncorrelated_scalar_subquery
{
- Self::collect_scalar_subqueries(logical_plan)
+ return self
+ .create_initial_plan_inner(logical_plan, session_state)
+ .await;
+ }
+
+ let mut all_subqueries = Self::collect_scalar_subqueries(logical_plan);
+ let freshened = if all_subqueries.iter().any(Subquery::is_volatile) {
+ Some(Self::freshen_volatile_subqueries(logical_plan)?)
} else {
- Vec::new()
+ None
};
+ let logical_plan = match &freshened {
+ Some(freshened) => {
+ all_subqueries = Self::collect_scalar_subqueries(freshened);
+ freshened
+ }
+ None => logical_plan,
+ };
Review Comment:
Calling `collect_scalar_subqueries` twice is unfortunate; as is the need to traverse the whole tree yet again to freshen the volatile SQs. I'm also not crazy about distinguishing between two volatile-containing subqueries based purely on the value of an Arc pointer.
Stepping back a bit, whether a SQ is volatile is not is a local property of the SQ, so we should be able to determine how a subquery should be handled as part of a single tree traversal. What if we did something like:
* Do a single pass over the plan
* When we encounter a subquery, determine if it is volatile or not
* If it's volatile, give it a fresh index. If non-volatile, look it up (structural equality) and assign/reuse an index as appropriate
* Write the index back into the logical plan node
That means that two textually equal SQs containing volatile expressions will compare non-equal via structural equality, so I think we'll get the semantics we want? It's a bit gross to write a field back into the logical plan but I can't think of something cleaner at the moment.
##########
datafusion/expr/src/expr.rs:
##########
@@ -2145,10 +2145,25 @@ impl Expr {
/// For example the function call `RANDOM()` is volatile as each call will
/// return a different value.
///
+ /// This also descends into subquery-bearing expressions, so
+ /// `(SELECT random())` is volatile even though the scalar subquery node is
+ /// not itself a volatile function.
+ ///
/// See [`Volatility`] for more information.
pub fn is_volatile(&self) -> bool {
- self.exists(|expr| Ok(expr.is_volatile_node()))
- .expect("exists closure is infallible")
+ self.exists(|expr| {
+ let subquery_is_volatile = match expr {
+ Expr::ScalarSubquery(subquery)
+ | Expr::Exists(Exists { subquery, .. })
+ | Expr::InSubquery(InSubquery { subquery, .. })
+ | Expr::SetComparison(SetComparison { subquery, .. }) => {
+ subquery.is_volatile()
+ }
+ _ => false,
+ };
+ Ok(expr.is_volatile_node() || subquery_is_volatile)
+ })
+ .expect("exists closure is infallible")
}
Review Comment:
```
$ ag is_volatile | wc -l
77
```
We should be careful about changing the semantics of a widely used API like this. It's not clear to me that the change is _wrong_ (or right), but it has both behavioral and performance consequences. We should look at all of those existing call-sites and understand whether recursing into subqueries is the right behavior or not. And if we are going to change this, we should ensure it has unit test coverage.
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#discussion_r3353837019
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -442,15 +474,30 @@ impl DefaultPhysicalPlanner {
// scalar subqueries to joins, so none should reach this point.
// Skip collection in that case to avoid creating a no-op
// `ScalarSubqueryExec` wrapper.
- let all_subqueries = if session_state
+ if !session_state
.config_options()
.optimizer
.enable_physical_uncorrelated_scalar_subquery
{
- Self::collect_scalar_subqueries(logical_plan)
+ return self
+ .create_initial_plan_inner(logical_plan, session_state)
+ .await;
+ }
+
+ let mut all_subqueries = Self::collect_scalar_subqueries(logical_plan);
+ let freshened = if all_subqueries.iter().any(Subquery::is_volatile) {
+ Some(Self::freshen_volatile_subqueries(logical_plan)?)
} else {
- Vec::new()
+ None
};
+ let logical_plan = match &freshened {
+ Some(freshened) => {
+ all_subqueries = Self::collect_scalar_subqueries(freshened);
+ freshened
+ }
+ None => logical_plan,
+ };
Review Comment:
Thank you @neilconway for great feedback as always. I have tried to address the concerns please check.
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "nathanb9 (via GitHub)" <gi...@apache.org>.
nathanb9 commented on code in PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#discussion_r3356010325
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4387,8 +4407,49 @@ impl Subquery {
subquery: plan,
outer_ref_columns: self.outer_ref_columns.clone(),
spans: Spans::new(),
+ scalar_subquery_index: self.scalar_subquery_index,
+ }
+ }
+
+ pub fn with_scalar_subquery_index(&self, index: SubqueryIndex) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: Some(index),
+ }
+ }
+
+ pub fn without_scalar_subquery_index(&self) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: None,
}
}
+
+ /// Returns `true` if this subquery's plan contains a volatile expression.
+ pub fn is_volatile(&self) -> bool {
+ plan_contains_volatile(&self.subquery)
+ }
+}
+
+/// Returns `true` if any expression in `plan` is volatile.
+fn plan_contains_volatile(plan: &LogicalPlan) -> bool {
Review Comment:
small but is it possible to make this public?
Would be useful for detecting volatile functions when deciding to materialize subplans (https://github.com/apache/datafusion/issues/22676)
--
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 volatile scalar subquery deduplication [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #22736:
URL: https://github.com/apache/datafusion/pull/22736#discussion_r3360236884
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -4387,8 +4407,49 @@ impl Subquery {
subquery: plan,
outer_ref_columns: self.outer_ref_columns.clone(),
spans: Spans::new(),
+ scalar_subquery_index: self.scalar_subquery_index,
+ }
+ }
+
+ pub fn with_scalar_subquery_index(&self, index: SubqueryIndex) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: Some(index),
+ }
+ }
+
+ pub fn without_scalar_subquery_index(&self) -> Subquery {
+ Subquery {
+ subquery: Arc::clone(&self.subquery),
+ outer_ref_columns: self.outer_ref_columns.clone(),
+ spans: self.spans.clone(),
+ scalar_subquery_index: None,
}
}
+
+ /// Returns `true` if this subquery's plan contains a volatile expression.
+ pub fn is_volatile(&self) -> bool {
+ plan_contains_volatile(&self.subquery)
+ }
+}
+
+/// Returns `true` if any expression in `plan` is volatile.
+fn plan_contains_volatile(plan: &LogicalPlan) -> bool {
Review Comment:
Sound good!
--
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