You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/10/01 12:44:07 UTC
(datafusion) branch main updated: test: close five mutation-testing gaps in leaf expression pushdown guards (#25452)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new 3fa5bc7130 test: close five mutation-testing gaps in leaf expression pushdown guards (#25452)
3fa5bc7130 is described below
commit 3fa5bc71304eecb2d59dc9271ed9e0975c06ace9
Author: Adrian Garcia Badaracco <17...@users.noreply.github.com>
AuthorDate: Thu Oct 1 12:32:44 2026 +0000
test: close five mutation-testing gaps in leaf expression pushdown guards (#25452)
## Which issue does this PR close?
- Part of https://github.com/apache/datafusion/issues/25451.
This PR closes 5 of the 14 real test gaps that the mutation run found.
It does not close the issue.
## Rationale for this change
`extract_leaf_expressions.rs` holds several guards that stop a rewrite
in one specific case. Each guard went in with a bug fix, and each fix
came with a plan snapshot test. A plan snapshot test asserts the shape
of one output plan. It does not assert that the guard makes its decision
for the reason the guard documents.
A `cargo-mutants` run over the file shows the result. Three guards let a
mutation through with no test failure:
| Function | Mutation that survived | Effect if this happened for real |
|---|---|---|
| `passthrough_column` | guard `col.name == alias.name` to `true` | A
renaming alias (`test.a AS b`) counts as a pass-through column. The
recovery projection that restores the name is then dropped, and the
rename is lost. |
| `is_pure_extraction_projection` | body to `true`, and the
`EXTRACTED_EXPR_PREFIX` guard to `true` | A projection that holds a
`__common_expr_1` alias is pushed down a second time. The expression is
re-extracted under a new alias that the parent cannot resolve. |
| `merge_would_duplicate_kept_expr` | `>` to `>=` | The guard fires on a
single reference site. Every merge into a child projection is then
blocked, and leaf extraction stops working over a projection. |
| `merge_would_duplicate_kept_expr` | `&&` to `\|\|` | Plain columns
count as reference sites. A merge that touches only plain columns is
blocked, although a plain column is cheap to duplicate. |
Each of these is a silent behaviour change. The suite stays green.
## What changes are included in this PR?
Three unit tests, one per function, in the existing `tests` module of
`datafusion/optimizer/src/extract_leaf_expressions.rs`. Each test calls
the guard directly and asserts both answers: the case where the guard
must fire, and the case where it must not. The file already uses this
style for `find_owning_input`.
- `test_passthrough_column_rejects_renaming_alias` covers a bare column,
a trivial rename that keeps the name, a rename that changes the name,
and an alias over a non-column expression.
- `test_is_pure_extraction_projection` covers a plan that is not a
projection, a columns-only projection, an extraction projection, a
projection that uses a `CommonSubexprEliminate` alias, and a projection
with a bare expression.
- `test_merge_would_duplicate_kept_expr_counts_only_kept_columns` covers
one reference site to a `KeepInPlace` column, two sites, one site plus a
pass-through reference, and two sites that reference a plain column
only. The pass-through case is the one that
https://github.com/apache/datafusion/issues/23655 reported.
The PR adds a small `keep_in_place_udf` test helper. It gives a
non-volatile `KeepInPlace` expression, which the existing `volatile_udf`
helper cannot.
There is no change to product code.
## What is the testing strategy for this PR?
This PR is tests only.
`cargo test --profile ci -p datafusion-optimizer` goes from 882 to 885
library tests. The 26 integration tests and 5 documentation tests are
unchanged. All pass.
To show that the new tests close the gaps, re-run `cargo-mutants` on the
three functions:
```bash
cargo mutants -p datafusion-optimizer \
-f 'datafusion/optimizer/src/extract_leaf_expressions.rs' \
-F 'passthrough_column|is_pure_extraction_projection|merge_would_duplicate_kept_expr' \
--profile ci --timeout 600 --jobs 2
```
| | Before | After |
|---|---|---|
| Caught | 16 | 21 |
| Missed | 5 | 0 |
| Unviable | 1 | 1 |
## Are there any user-facing changes?
No.
Note: the test for `merge_would_duplicate_kept_expr` becomes dead when
https://github.com/apache/datafusion/pull/25456 lands, because that PR
deletes the function. Whichever merges second drops that one test; the
other two tests apply unchanged.
Part of the leaf-pushdown EPIC:
https://github.com/apache/datafusion/issues/25459
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Fable 5.1 <no...@anthropic.com>
---
.../optimizer/src/extract_leaf_expressions.rs | 161 +++++++++++++++++++++
1 file changed, 161 insertions(+)
diff --git a/datafusion/optimizer/src/extract_leaf_expressions.rs b/datafusion/optimizer/src/extract_leaf_expressions.rs
index 8a4abfcb48..e38d89a467 100644
--- a/datafusion/optimizer/src/extract_leaf_expressions.rs
+++ b/datafusion/optimizer/src/extract_leaf_expressions.rs
@@ -3601,4 +3601,165 @@ mod tests {
Ok(())
}
+
+ /// `passthrough_column` tells `split_and_push_projection` which projection
+ /// expressions the input already produces. A renaming alias (`test.a AS b`)
+ /// does not qualify: it is a new output column. If the function accepted
+ /// it, the rename would count as captured, and the recovery projection
+ /// that restores the name would be dropped.
+ #[test]
+ fn test_passthrough_column_rejects_renaming_alias() {
+ // A bare column is a pass-through.
+ let bare = Expr::Column(Column::new(Some("test"), "a"));
+ assert_eq!(
+ passthrough_column(&bare).map(|c| c.flat_name()),
+ Some("test.a".to_string())
+ );
+
+ // `test.a AS a` keeps the name, so it is a pass-through.
+ let trivial_rename = Expr::Column(Column::new(Some("test"), "a")).alias("a");
+ assert_eq!(
+ passthrough_column(&trivial_rename).map(|c| c.flat_name()),
+ Some("test.a".to_string())
+ );
+
+ // `test.a AS b` changes the name, so it is not a pass-through.
+ let renaming = Expr::Column(Column::new(Some("test"), "a")).alias("b");
+ assert_eq!(passthrough_column(&renaming), None);
+
+ // An alias over anything other than a column is not a pass-through.
+ let not_a_column = lit(1).alias("a");
+ assert_eq!(passthrough_column(¬_a_column), None);
+ }
+
+ /// `is_pure_extraction_projection` decides if a merged projection can go
+ /// down one more level. It must accept only `__datafusion_extracted`
+ /// aliases and plain columns, and only with at least one extraction. An
+ /// alias with a different prefix must not count: a second push would
+ /// re-extract the expression under a new alias that the parent cannot
+ /// resolve.
+ #[test]
+ fn test_is_pure_extraction_projection() -> Result<()> {
+ let scan = test_table_scan_with_struct()?;
+
+ // Not a projection.
+ assert!(!is_pure_extraction_projection(&scan));
+
+ // Columns only: nothing was extracted.
+ let columns_only = LogicalPlanBuilder::from(scan.clone())
+ .project(vec![col("id"), col("user")])?
+ .build()?;
+ assert!(!is_pure_extraction_projection(&columns_only));
+
+ // One extraction alias and one pass-through column.
+ let pure = LogicalPlanBuilder::from(scan.clone())
+ .project(vec![
+ leaf_udf(col("user"), "status").alias("__datafusion_extracted_1"),
+ col("id"),
+ ])?
+ .build()?;
+ assert!(is_pure_extraction_projection(&pure));
+
+ // The same shape under a `CommonSubexprEliminate` alias is not an
+ // extraction projection.
+ let common_expr = LogicalPlanBuilder::from(scan.clone())
+ .project(vec![
+ leaf_udf(col("user"), "status").alias("__common_expr_1"),
+ col("id"),
+ ])?
+ .build()?;
+ assert!(!is_pure_extraction_projection(&common_expr));
+
+ // A bare expression is never allowed.
+ let bare_expr = LogicalPlanBuilder::from(scan)
+ .project(vec![leaf_udf(col("user"), "status"), col("id")])?
+ .build()?;
+ assert!(!is_pure_extraction_projection(&bare_expr));
+
+ Ok(())
+ }
+
+ /// A non-volatile `KeepInPlace` expression: the kind that
+ /// `CommonSubexprEliminate` hoists into a column so it runs one time.
+ fn keep_in_place_udf(expr: Expr) -> Expr {
+ Expr::ScalarFunction(ScalarFunction::new_udf(
+ Arc::new(ScalarUDF::new_from_impl(
+ PlacementTestUDF::new().with_placement(ExpressionPlacement::KeepInPlace),
+ )),
+ vec![expr],
+ ))
+ }
+
+ /// `merge_would_duplicate_kept_expr` must block a merge only when it would
+ /// inline one `KeepInPlace` definition into more than one reference site.
+ /// It must count reference sites for `KeepInPlace` columns alone: a plain
+ /// column is cheap to duplicate, and a merge that touches only plain
+ /// columns must still go ahead.
+ #[test]
+ fn test_merge_would_duplicate_kept_expr_counts_only_kept_columns() -> Result<()> {
+ let scan = test_table_scan_with_struct()?;
+ // `k` holds a `KeepInPlace` definition. `id` is a plain column.
+ let child = Projection::try_new(
+ vec![keep_in_place_udf(col("user")).alias("k"), col("id")],
+ Arc::new(scan),
+ )?;
+ let no_columns = IndexSet::new();
+
+ // One reference site: the merge inlines one copy, so it is free.
+ let one_site = vec![(
+ get_field_like(col("k"), "a"),
+ "__datafusion_extracted_1".to_string(),
+ )];
+ assert!(!merge_would_duplicate_kept_expr(
+ &one_site,
+ &no_columns,
+ &child
+ ));
+
+ // Two reference sites: the merge would evaluate `k` two times.
+ let two_sites = vec![
+ (
+ get_field_like(col("k"), "a"),
+ "__datafusion_extracted_1".to_string(),
+ ),
+ (
+ get_field_like(col("k"), "b"),
+ "__datafusion_extracted_2".to_string(),
+ ),
+ ];
+ assert!(merge_would_duplicate_kept_expr(
+ &two_sites,
+ &no_columns,
+ &child
+ ));
+
+ // A pass-through reference to `k` is a second site too (#23655).
+ let kept_column: IndexSet<Column> =
+ std::iter::once(Column::new_unqualified("k")).collect();
+ assert!(merge_would_duplicate_kept_expr(
+ &one_site,
+ &kept_column,
+ &child
+ ));
+
+ // Two sites that reference the plain column `id` are cheap to
+ // duplicate, so the merge must still go ahead.
+ let plain_column_sites = vec![
+ (
+ get_field_like(col("id"), "a"),
+ "__datafusion_extracted_1".to_string(),
+ ),
+ (
+ get_field_like(col("id"), "b"),
+ "__datafusion_extracted_2".to_string(),
+ ),
+ ];
+ assert!(!merge_would_duplicate_kept_expr(
+ &plain_column_sites,
+ &no_columns,
+ &child
+ ));
+
+ Ok(())
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org