Skip to content

fix: evaluate struct-returning UDFs once across repeated field accesses#23691

Open
tohuya6 wants to merge 1 commit into
apache:mainfrom
tohuya6:fix-23655-redundant-struct-udf
Open

fix: evaluate struct-returning UDFs once across repeated field accesses#23691
tohuya6 wants to merge 1 commit into
apache:mainfrom
tohuya6:fix-23655-redundant-struct-udf

Conversation

@tohuya6

@tohuya6 tohuya6 commented Jul 18, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Closes #23655.

Rationale for this change

SELECT f(c)['a'], f(c)['b'], where f is a struct-returning UDF, evaluates f(c) twice in the physical ProjectionExec — once per get_field — although the two calls are identical.

CommonSubexprEliminate already hoists the shared f(c) into a __common_expr_N column so it runs once. PushDownLeafProjections then undoes that, one pass at a time:

  • After CSE, each field access reads its base from the hoist column, so get_field's placement flips to MoveTowardsLeafNodes and the pass extracts it.
  • Merging the extraction into the hoist projection inlines that column's definition back into every access (via build_projection_replace_map), re-duplicating the KeepInPlace UDF.
  • optimize_projections drops the now-unused hoist column, and the two passes oscillate pass after pass — settling on a double evaluation.

This is the instability the is_ignored note in common_subexpr_eliminate already warns about.

What changes are included in this PR?

Adds a guard merge_would_duplicate_kept_expr and calls it from split_and_push_projection, which now bails out — leaving the projection untouched — when merging the extractions into the input projection would inline a KeepInPlace definition that more than one extraction pair references. This mirrors the existing optimize_projections merge guard (#8296): a compute-once expression referenced more than once is kept in place instead of duplicated.

With the guard, CSE's hoist survives, optimize_projections keeps it, and the pipeline converges to a single evaluation:

Before:

Projection: get_field(f(c), 'a'), get_field(f(c), 'b')   -- f(c) twice
  TableScan

After:

Projection: get_field(__common_expr_1, 'a'), get_field(__common_expr_1, 'b')
  Projection: f(c) AS __common_expr_1                      -- f(c) once
    TableScan

Are these changes tested?

Yes.

  • test_struct_returning_udf_evaluated_once (in extract_leaf_expressions.rs) runs the four interacting rules — CSE → extraction → pushdown → projection pruning — and asserts no field access wraps a fresh UDF call, plus an insta snapshot of the single-eval plan. It runs with the default pass budget, so any residual oscillation would surface as a double evaluation.
  • get_field_like — a small test UDF added to test/udfs.rs — reproduces GetFieldFunc's conditional placement (MoveTowardsLeafNodes only when the base is pushable and the keys are literals). This lets the optimizer crate model struct-field access without depending on datafusion-functions, which it intentionally does not.

cargo fmt --all and cargo clippy --all-targets --all-features -- -D warnings are clean.

Are there any user-facing changes?

Yes, but only in the sense that a redundantly-evaluated path becomes single-evaluation. No API or query-result changes: affected plans evaluate a struct-returning UDF once instead of once per field access, a saving that scales with the number of distinct field accesses on the same UDF.

## Which issue does this PR close?

Closes apache#23655.

## Rationale for this change

`SELECT f(c)['a'], f(c)['b']`, where `f` is a struct-returning UDF, evaluates `f(c)` twice in the physical `ProjectionExec` — once per `get_field` — although the two calls are identical.

`CommonSubexprEliminate` already hoists the shared `f(c)` into a `__common_expr_N` column so it runs once. `PushDownLeafProjections` then undoes that, one pass at a time:

- After CSE, each field access reads its base from the hoist column, so `get_field`'s placement flips to `MoveTowardsLeafNodes` and the pass extracts it.
- Merging the extraction into the hoist projection inlines that column's definition back into every access (via `build_projection_replace_map`), re-duplicating the `KeepInPlace` UDF.
- `optimize_projections` drops the now-unused hoist column, and the two passes oscillate pass after pass — settling on a double evaluation.

This is the instability the `is_ignored` note in `common_subexpr_eliminate` already warns about.

## What changes are included in this PR?

`split_and_push_projection` now bails out — leaving the projection untouched — when merging the extractions into the input projection would inline a `KeepInPlace` definition that more than one extraction pair references. This mirrors the existing `optimize_projections` merge guard (apache#8296): a compute-once expression referenced more than once is kept in place instead of duplicated.

With the guard, CSE's hoist survives, `optimize_projections` keeps it, and the pipeline converges to a single evaluation:

Before:
```text
Projection: get_field(f(c), 'a'), get_field(f(c), 'b')   -- f(c) twice
  TableScan
```

After:
```text
Projection: get_field(__common_expr_1, 'a'), get_field(__common_expr_1, 'b')
  Projection: f(c) AS __common_expr_1                      -- f(c) once
    TableScan
```

## Are these changes tested?

Yes.

- `test_struct_returning_udf_evaluated_once` (in `extract_leaf_expressions.rs`) runs the four interacting rules — CSE → extraction → pushdown → projection pruning — and asserts no field access wraps a fresh UDF call, plus an `insta` snapshot of the single-eval plan. It runs with the default pass budget, so any residual oscillation would surface as a double evaluation.
- `get_field_like` — a small test UDF added to `test/udfs.rs` — reproduces `GetFieldFunc`'s conditional placement (`MoveTowardsLeafNodes` only when the base is pushable and the keys are literals). This lets the optimizer crate model struct-field access without depending on `datafusion-functions`, which it intentionally does not.

`cargo fmt --all` and `cargo clippy --all-targets --all-features -- -D warnings` are clean.

## Are there any user-facing changes?

No API changes. Affected plans evaluate a struct-returning UDF once instead of once per field access; query results are unchanged. The saving scales with the number of distinct field accesses on the same UDF.

## Follow-ups

- An end-to-end `.slt` test would need a struct-returning UDF registered in the sqllogictest harness: the built-in `named_struct` / `struct` constructors are folded into their fields by the field-over-struct-literal rewrite before this interaction can occur, so only an opaque (UDF) struct source reproduces it. Covered at the optimizer layer here instead.
@github-actions github-actions Bot added the optimizer Optimizer rules label Jul 18, 2026
@tohuya6
tohuya6 marked this pull request as ready for review July 18, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize redundant calls to UDFs producing StructArray

1 participant