fordN commented on code in PR #23395:
URL: https://github.com/apache/datafusion/pull/23395#discussion_r3553007706
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -649,6 +650,45 @@ fn project_output_partitioning(
}
}
+/// Returns `true` if merging `outer` into `inner` would duplicate a volatile
+/// expression; the caller should then decline the merge.
+///
+/// `inner` is the scan's current projection and `outer` the projection being
+/// pushed into it; merging substitutes each `inner` expression into every
+/// `outer` reference to it. If a volatile `inner` expression (e.g. `random()`,
+/// `uuid()`) is referenced by `outer`, that one value gets inlined at each
site
+/// and re-evaluated independently, so references meant to share a single
+/// "locked-in" value diverge. This is the volatility guard the physical
+/// `ProjectionPushdown` and `FilterPushdown` rules already apply (see
+/// `datafusion_physical_expr_common::physical_expr::is_volatile`).
+///
+/// A volatile `inner` expression blocks the merge whenever `outer` references
+/// it at all: `collect_columns` reports a column at most once per `outer`
+/// expression, so a self-duplicating expression such as `r + r` would slip
past
+/// a stricter `> 1` threshold.
+fn would_duplicate_volatile_exprs(
+ inner: &ProjectionExprs,
+ outer: &ProjectionExprs,
+) -> bool {
+ let inner_exprs = inner.as_ref();
+ let outer_exprs = outer.as_ref();
+
+ let mut referenced = vec![false; inner_exprs.len()];
+ for proj_expr in outer_exprs {
+ for col in collect_columns(&proj_expr.expr) {
+ let idx = col.index();
+ if idx < referenced.len() {
+ referenced[idx] = true;
+ }
+ }
+ }
+
+ referenced
+ .iter()
+ .enumerate()
+ .any(|(idx, &referenced)| referenced &&
is_volatile(&inner_exprs[idx].expr))
Review Comment:
@xudong963 good call. I've updated the reference counting algorithm to
count references with multiplicity similar to how it's done in
`try_collapse_projection_chain()`. Now with more precise counting we can block
only when volatile expression is referenced more than once.
I've also added a new unit test for the single-reference case.
--
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]
