Skip to content

fix: identify optimizer aliases with typed origins - #26769

Closed
mattsu2020 wants to merge 1 commit into
apache:mainfrom
mattsu2020:25449
Closed

mattsu2020 wants to merge 1 commit into
apache:mainfrom
mattsu2020:25449

Conversation

@mattsu2020

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A user alias such as __datafusion_extracted_1 can currently be mistaken for an optimizer-generated extraction. This lets an ordinary column name change how optimizer rules treat the expression. Plans built through the public API or restored from serialization have the same ambiguity.

What changes are included in this PR?

  • Add AliasOrigin::{User, CommonSubexpr, LeafExtraction}. Mark generated definitions explicitly and use the origin to recognize leaf extractions.
  • Preserve origins through expression traversal, coercion, name restoration, and projection merging. Include the origin in alias equality, ordering, and hashing.
  • Add an optional protobuf origin field and regenerate the Rust/JSON models. Missing origins default to User; names are never used to infer the origin. Older readers can discard the new field.
  • Keep displayed alias names and the existing collision-prevention scan. Origins do not change name-based column resolution, so a fresh alias generator still needs to advance past existing names, including aliases inside subqueries.
  • Box alias field metadata to retain the existing Expr and LogicalPlan sizes of 112 and 176 bytes.
  • Update extraction fixtures to set their origins explicitly and update two SQL plan expectations where user aliases are now handled as user aliases.

SQL cannot encode the marker. The unparser continues to emit ordinary aliases, and reparsing gives them User origins. This PR tests leaf-extraction SQL round trips; it retains the existing SQL reservation of __common_expr names.

What is the testing strategy for this PR?

Regression coverage includes:

  • User aliases with the extraction prefix, and extraction markers with arbitrary names.
  • Origin preservation during expression rewrites and protobuf binary/JSON round trips.
  • Backward-compatible decoding of aliases without an origin.
  • Re-optimizing a serialized optimized plan with a fresh context.
  • Unparsing, replanning, and executing a leaf-extraction plan containing a user alias named __datafusion_extracted_1.

Validation passed:

  • cargo fmt --all and the final formatting check.
  • cargo clippy --all-targets --all-features -- -D warnings.
  • The workspace extended test command below: 12,990 Rust tests passed, 8 ignored, and all 542 SQL test files passed.
  • A separate run of projection_pushdown.slt.
RUST_BACKTRACE=1 cargo test --profile ci \
    --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli \
    --workspace --lib --tests --bins \
    --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption

Compared the five existing optimize_projections benchmark cases against Apache main at 350ca76505c9513d16e7dd9910bcd9bde2c911ed, using the ci profile and sequential measurements without concurrent builds/tests. Point estimates ranged from -5.2% to +1.0%. These are unoptimized-profile measurements.

Are there any user-facing changes?

User aliases beginning with __datafusion_extracted are no longer identified as optimizer-generated solely from their names. Output column names remain intact.

Public API changes:

  • Alias gains an origin field and with_origin(); existing alias constructors default to User.
  • Alias.metadata changes from Option<FieldMetadata> to Option<Box<FieldMetadata>>. The with_metadata() argument type remains unchanged.
  • SavedName::Saved gains an origin field.

The protobuf addition is optional, and SQL reserved-name validation is unchanged.

@github-actions github-actions Bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) proto Related to proto crate labels Oct 26, 2026
@github-actions

Copy link
Copy Markdown

Thank you for opening this pull request!

Reviewer note: 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
     Cloning 350ca76505c9513d16e7dd9910bcd9bde2c911ed
    Building datafusion v55.1.0 (current)
       Built [  63.438s] (current)
     Parsing datafusion v55.1.0 (current)
      Parsed [   0.036s] (current)
    Building datafusion v55.1.0 (baseline)
       Built [  17.752s] (baseline)
     Parsing datafusion v55.1.0 (baseline)
      Parsed [   0.038s] (baseline)
    Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.635s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  83.451s] datafusion
    Building datafusion-expr v55.1.0 (current)
       Built [  22.923s] (current)
     Parsing datafusion-expr v55.1.0 (current)
      Parsed [   0.108s] (current)
    Building datafusion-expr v55.1.0 (baseline)
       Built [   7.552s] (baseline)
     Parsing datafusion-expr v55.1.0 (baseline)
      Parsed [   0.111s] (baseline)
    Checking datafusion-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.409s] 229 checks: 227 pass, 2 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field Alias.origin in /home/runner/work/datafusion/datafusion/datafusion/expr/src/expr.rs:740

--- failure enum_struct_variant_field_added: pub enum struct variant field added ---

Description:
An enum's exhaustive struct variant has a new field, which has to be included when constructing or matching on this variant.
        ref: https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/enum_struct_variant_field_added.ron

Failed in:
  field origin of variant SavedName::Saved in /home/runner/work/datafusion/datafusion/datafusion/expr/src/expr_rewriter/mod.rs:352

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  32.917s] datafusion-expr
    Building datafusion-optimizer v55.1.0 (current)
       Built [  17.431s] (current)
     Parsing datafusion-optimizer v55.1.0 (current)
      Parsed [   0.036s] (current)
    Building datafusion-optimizer v55.1.0 (baseline)
       Built [   7.712s] (baseline)
     Parsing datafusion-optimizer v55.1.0 (baseline)
      Parsed [   0.038s] (baseline)
    Checking datafusion-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.166s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  26.068s] datafusion-optimizer
    Building datafusion-physical-expr v55.1.0 (current)
       Built [   3.750s] (current)
     Parsing datafusion-physical-expr v55.1.0 (current)
      Parsed [   0.059s] (current)
    Building datafusion-physical-expr v55.1.0 (baseline)
       Built [   3.741s] (baseline)
     Parsing datafusion-physical-expr v55.1.0 (baseline)
      Parsed [   0.062s] (baseline)
    Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.377s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [   8.712s] datafusion-physical-expr
    Building datafusion-proto v55.1.0 (current)
       Built [  29.222s] (current)
     Parsing datafusion-proto v55.1.0 (current)
      Parsed [   0.013s] (current)
    Building datafusion-proto v55.1.0 (baseline)
       Built [  22.030s] (baseline)
     Parsing datafusion-proto v55.1.0 (baseline)
      Parsed [   0.014s] (baseline)
    Checking datafusion-proto v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.144s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  52.603s] datafusion-proto
    Building datafusion-proto-models v55.1.0 (current)
       Built [  18.318s] (current)
     Parsing datafusion-proto-models v55.1.0 (current)
      Parsed [   0.172s] (current)
    Building datafusion-proto-models v55.1.0 (baseline)
       Built [   6.257s] (baseline)
     Parsing datafusion-proto-models v55.1.0 (baseline)
      Parsed [   0.175s] (baseline)
    Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   1.973s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field AliasNode.origin in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1048
  field AliasNode.origin in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1048

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  28.235s] datafusion-proto-models
    Building datafusion-sql v55.1.0 (current)
       Built [  20.487s] (current)
     Parsing datafusion-sql v55.1.0 (current)
      Parsed [   0.034s] (current)
    Building datafusion-sql v55.1.0 (baseline)
       Built [  12.874s] (baseline)
     Parsing datafusion-sql v55.1.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion-sql v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.275s] 229 checks: 229 pass, 31 skip
     Summary no semver update required
    Finished [  34.597s] datafusion-sql

@alamb

alamb commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution! I am closing this under DataFusion's AI Dump policy.

https://datafusion.apache.org/contributor-guide/ai-policy.html#what-is-an-ai-dump-and-why-it-is-not-helpful

This has the hallmarks of AI code that was not reviewed by the author: one of three sizable PRs opened the same day after claiming three unrelated issues, with a template validation section listing exact test counts and a new enum threaded across expr, optimizer and proto without prior design discussion.

If you would like to continue, please open a focused PR in your own words that you have reviewed and understand, and we are happy to help with review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto detected api change Auto detected API change core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates proto Related to proto crate sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace the __datafusion_extracted / __common_expr alias prefix protocol with a typed marker

2 participants