Repository navigation
fix: infer UNION output types before analysis - #27139
Open
kumarUjjawal wants to merge 1 commit into
Open
kumarUjjawal wants to merge 1 commit into
kumarUjjawal wants to merge 1 commit into
Conversation
Member
|
I was just about to post a fix too 😭 |
Member
|
Should've taken the issue yesterday |
alamb
approved these changes
Nov 5, 2026
| /// alone is not enough here -- it reflects the raw, pre-`Analyzer` plan (see | ||
| /// `SessionState::create_logical_plan`), which for a `UNION` still has the | ||
| /// first leg's un-coerced type. | ||
| /// This test checks the schema after the Analyzer and Optimizer run, |
Contributor
There was a problem hiding this comment.
thank you for trimming the AI verbosity
| in this section pertains to features and changes that have already been merged | ||
| to the main branch and are awaiting release in this version. | ||
|
|
||
| ### UNION and CASE infer common output types before analysis |
Contributor
There was a problem hiding this comment.
This is a bug fix. Why does it need a upgrade guide entry?
| `datafusion_expr::type_coercion::other::coerce_union_type` is a new helper | ||
| that infers one UNION column type from input fields. | ||
|
|
||
| For `UNION BY NAME`, missing columns participate as `NULL` during type |
Contributor
There was a problem hiding this comment.
If we want to document the behavior of how schema resolution works, it should be in the code documentation I think
| .transpose()?; | ||
| // If the branches have no common type, keep the old provisional | ||
| // type. The analyzer reports the error. | ||
| Ok( |
Contributor
There was a problem hiding this comment.
we could probably make this more concse / less indented with some intermediate variables
| pub fn coerce_union_schema(inputs: &[Arc<LogicalPlan>]) -> Result<DFSchema> { | ||
| coerce_union_schema_with_schema(&inputs[1..], inputs[0].schema()) | ||
| let Some(first) = inputs.first() else { | ||
| return plan_err!("UNION requires at least one input"); |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn union_coercion_preserves_values() -> Result<()> { |
Contributor
There was a problem hiding this comment.
I feel like most of these tests can be written as .slt tests (but better). I recommend leaving some tests here that cover the expected schema case but don't run the query -- instead run the queries as a .slt tests
Contributor
|
Thank you for running with this @kumarUjjawal |
sdf-jkl
added a commit
to sdf-jkl/datafusion
that referenced
this pull request
Nov 5, 2026
Incorporate deferred CASE/UNION coercion errors, dictionary coercion from actual inputs, and an empty-input guard, informed by: apache#27139 Keep regression coverage in SQL and document the dictionary output-type change for UNION BY NAME.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
DataFrame::schema()previously reported the first UNION input's type while execution used the common type. UNION construction now infers the common output type.What changes are included in this PR?
coerce_union_typehelper.Synthetic nested CASE planning (
ci-bench, SQL through physical planning):These three points suggest roughly quadratic extra work. Extrapolating that trend to depth 1,000 gives about 45 ms of extra planning. The benchmark did not measure depth 1,000.
What is the testing strategy for this PR?
The extended suite (13,511 Rust tests), all 547 SLT files, full lint suite, all-features Clippy, and focused regressions pass.
Are there any user-facing changes?
The upgrade note in
docs/source/library-user-guide/upgrading/56.0.0.mdcovers the change ofxfromDictionary(Int32, Utf8)toUtf8in this query and the other inference changes.