fix: align CrossJoinExec schema metadata merge with the logical plan#23442
fix: align CrossJoinExec schema metadata merge with the logical plan#23442fangchenli wants to merge 1 commit into
Conversation
`CrossJoinExec::new` merged schema-level metadata right-biased (`left.metadata().extend(right)`), while the logical plan and the shared physical `build_join_schema` are left-biased for inner joins. When both inputs carry the same metadata key with different values, the cross join's physical schema diverged from the logical one and tripped the physical planner's `schema_satisfied_by` check (raised when the cross join feeds an aggregate), failing with an internal error. Route `CrossJoinExec::new` through the shared `build_join_schema` helper (the one apache#16221 fixed for hash/nested-loop joins but never applied to cross join) so metadata is merged consistently. Field output is unchanged for inner joins. Adds a unit test and a metadata.slt regression over two tables whose schema metadata conflicts under the same key. Closes apache#23434 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the change. The core fix looks right: for JoinType::Inner the shared build_join_schema produces a byte-identical field list to the old hand-rolled code, so only the metadata precedence changes to left-wins, matching the logical plan. Proto roundtrip re-derives the schema via CrossJoinExec::new, so no compat issue there.
A few comments inline, one about a remaining gap worth documenting.
FYI: AI assisted comments - but ive reviewed / verified each 👌
| // Use the shared helper (inner join) so metadata merges the same way as | ||
| // the logical plan; merging it here independently let schemas diverge. | ||
| let (schema, _) = | ||
| build_join_schema(&left.schema(), &right.schema(), &JoinType::Inner); |
There was a problem hiding this comment.
This aligns the initial plan, but the swap path still diverges: swap_inputs (line 188) rebuilds via CrossJoinExec::new(right, left), and with left-wins semantics the metadata winner flips under swap. OptimizationInvariantChecker then rejects the rule output. Running the issue's tables through SELECT * FROM t_left CROSS JOIN t_right on this branch:
Internal error: PhysicalOptimizer rule 'join_selection' failed. Schema mismatch. Expected original schema: Field { "a": Int32 }, Field { "b": Int32 }, got new schema: Field { "a": Int32 }, Field { "b": Int32 }.
(The printed schemas look identical because only fields are displayed; the difference is the schema-level metadata.) Setting datafusion.optimizer.join_reordering = false makes it pass, so it is the swap. The same flip exists for HashJoin/NLJ Inner and Full swaps on main, so it is pre-existing and arguably out of scope, but it would be good to note it in the PR description as a known remaining gap, or fix reorder_output_after_swap to re-impose the original schema metadata on the reverting projection. Happy to file a follow-up issue.
| # count(DISTINCT ...) keeps a real cross join (plain count folds from stats). | ||
| # See https://github.com/apache/datafusion/issues/23434 | ||
| query I | ||
| SELECT count(DISTINCT "l"."id") |
There was a problem hiding this comment.
The aggregate on top masks the swap path (see the comment on cross_join.rs), so this test cannot catch the join_selection failure mode. Consider also adding a SELECT * variant with reordering disabled to pin the fix directly:
statement ok
set datafusion.optimizer.join_reordering = false;
query II
SELECT "l"."id", "r"."id"
FROM "table_with_metadata" AS "l", "table_with_metadata_alt" AS "r"
ORDER BY "l"."id", "r"."id";| // Use the shared helper (inner join) so metadata merges the same way as | ||
| // the logical plan; merging it here independently let schemas diverge. | ||
| let (schema, _) = | ||
| build_join_schema(&left.schema(), &right.schema(), &JoinType::Inner); |
There was a problem hiding this comment.
nit: PR #16221 added test_join_metadata in both joins/utils.rs and the logical builder.rs to pin the left-wins/right-wins convention per join type. A matching unit test in cross_join.rs (construct a CrossJoinExec from two inputs with conflicting schema metadata, assert output metadata equals the left input's value) would be cheap to add and guards against future drift without needing the full planning pipeline.
Which issue does this PR close?
Rationale for this change
A cross join over two inputs whose schemas share a metadata key with different
values produced a physical schema that disagreed with the logical plan, so when
the cross join fed an aggregate the planner failed with "Physical input schema
should be the same as the one converted from logical input schema".
The logical plan and the shared
build_join_schemahelper merge inner-joinmetadata left-biased, but
CrossJoinExec::newmerged its own schema right-biased(
left.metadata().extend(right)). #16221 fixed the shared helper for hash andnested-loop joins but never touched cross join.
What changes are included in this PR?
Route
CrossJoinExec::newthrough the sharedbuild_join_schemahelper so itsmetadata merges the same way as the logical plan. Field output is unchanged for
inner joins.
Are these changes tested?
Yes, a
metadata.sltregression that cross-joins two tables with conflictingschema metadata into an aggregate. It failed with the internal error before and
passes after.
Are there any user-facing changes?
Bug fix only: affected cross joins now plan and run instead of erroring. No API
changes.