Skip to content

refactor: resolve columns by index in the leaf-pushdown rules - #26871

Draft
adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:refactor/leaf-rules-resolve-columns-by-index
Draft

adriangb wants to merge 2 commits into
apache:mainfrom
adriangb:refactor/leaf-rules-resolve-columns-by-index

Conversation

@adriangb

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

The leaf-pushdown rules (ExtractLeafExpressions, PushDownLeafProjections) match columns by name. A bare c and a qualified t.c can name the same field but do not compare equal. Two fields with the same name can be confused.

One result: when a merge into an existing projection looks for an extraction that is already there, the lookup misses if the two spell the column differently. The rule then adds the same extraction a second time. With the same alias, Projection::try_new fails with DuplicateUnqualifiedField. This is the qualified/unqualified path of #25447.

What changes are included in this PR?

One resolver, column_index(schema, col) -> Option<usize> (None when the column is missing or ambiguous). All decisions below compare indices.

Site Before After
remap_pairs_and_columns (SubqueryAlias, Union) replace_cols_by_name with a qualified_name map remap by position
Merge: resolve through the projection build_projection_replace_map (HashMap<String, Expr>) projection_definition(proj, idx), by output index
Merge: find an existing extraction Expr equality, as spelled (Expr, alias) set, Expr in the canonical spelling of the input schema
Merge: pass-through columns name sets (output_names, existing_cols) column_index in the output schema, then has_column_with_unqualified_name
resolve_against qualified_field_from_column (first match) column_index (rejects ambiguous)
volatile_output_columns / would_duplicate_volatile flat_name sets index sets
merge_would_duplicate_kept_expr flat_name counts index counts

The merge also no longer adds an extraction a second time when the existing projection already computes it under two aliases.

Still by name (follow-ups):

What is the testing strategy for this PR?

  • New unit tests in extract_leaf_expressions.rs:
    • test_merge_finds_extraction_with_other_column_spelling: existing projection has leaf_udf(a) AS __datafusion_extracted_1, caller asks for leaf_udf(test.a) under the same alias. Fails on main with DuplicateUnqualifiedField, passes here.
    • test_merge_finds_extraction_computed_under_two_aliases.
  • One snapshot changes: in the merge test with test.c + 1 AS d, the extraction now prints as leaf_udf(test.a, ...) instead of leaf_udf(a, ...). The merge now writes columns in the canonical spelling of the input schema. Same plan, same result.
  • No .slt changes.
  • cargo test -p datafusion-optimizer, all sqllogictests, and the leaf-pushdown differential fuzz test (LEAF_PUSHDOWN_FUZZ_CASES=2000) pass.

Are there any user-facing changes?

No. Internal refactor of two optimizer rules. A merge that hits the qualified/unqualified mismatch no longer adds a duplicate extraction.

🤖 Generated with Claude Code

Replace name-based column matching (replace_cols_by_name, flat_name keys)
in ExtractLeafExpressions / PushDownLeafProjections with a single index
resolver. A bare `c` and a qualified `t.c` that name the same field now
compare equal, so the merge finds an existing extraction spelled the
other way instead of adding it a second time.

Part of apache#25448.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
adriangb added a commit to adriangb/datafusion that referenced this pull request Oct 29, 2026
Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.

1 participant