nathanb9 commented on code in PR #23442:
URL: https://github.com/apache/datafusion/pull/23442#discussion_r3560036330
##########
datafusion/physical-plan/src/joins/cross_join.rs:
##########
@@ -102,23 +102,11 @@ pub struct CrossJoinExec {
impl CrossJoinExec {
/// Create a new [CrossJoinExec].
pub fn new(left: Arc<dyn ExecutionPlan>, right: Arc<dyn ExecutionPlan>) ->
Self {
- // left then right
- let (all_columns, metadata) = {
- let left_schema = left.schema();
- let right_schema = right.schema();
- let left_fields = left_schema.fields().iter();
- let right_fields = right_schema.fields().iter();
-
- let mut metadata = left_schema.metadata().clone();
- metadata.extend(right_schema.metadata().clone());
-
- (
- left_fields.chain(right_fields).cloned().collect::<Fields>(),
- metadata,
- )
- };
-
- let schema =
Arc::new(Schema::new(all_columns).with_metadata(metadata));
+ // 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);
Review 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.
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -124,6 +124,16 @@ FROM
----
6
+# Regression test: cross join over two tables with conflicting schema metadata,
+# feeding an aggregate, used to fail the physical planner's schema check.
+# 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")
Review 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:
```sql
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";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
