fix: Preserve metadata when a cross-join is swapped#23605
Conversation
5176f86 to
8fe3468
Compare
| pub fn swap_inputs(&self) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let new_join = | ||
| CrossJoinExec::new(Arc::clone(&self.right), Arc::clone(&self.left)); | ||
| // Rebuild schema with columns from right to left, preserve existing metadata |
There was a problem hiding this comment.
This rebuilds the schema in the same way as CrossJoinExec::new does and preserves the metadata.
There was a problem hiding this comment.
I wonder if we can make a helper (as a follow on) to handle this logic rather than sprinkling the same fix through the code in many places 🤔
There was a problem hiding this comment.
Do you want the helper as part of this PR?
There was a problem hiding this comment.
We can do it as a follow on PR perhaps
|
FYI @fangchenli |
| pub fn swap_inputs(&self) -> Result<Arc<dyn ExecutionPlan>> { | ||
| let new_join = | ||
| CrossJoinExec::new(Arc::clone(&self.right), Arc::clone(&self.left)); | ||
| // Rebuild schema with columns from right to left, preserve existing metadata |
There was a problem hiding this comment.
I wonder if we can make a helper (as a follow on) to handle this logic rather than sprinkling the same fix through the code in many places 🤔
|
@alamb Thanks for the review! |
|
Thanks @mkleen |
|
Do we have an end-to-end bug reproducer (SQL/query level)? This looks like a major bug, and I think the given UT can't guarantee the test goal, here is the reason: #23759 (comment) I can help to look into it further. |
@2010YOUY01 See #23857 |
Which issue does this PR close?
join_selectionflips schema-level metadata, failing "Schema mismatch" #23461 but resolves it only for cross-joins. The same problem exisists for Hash/NL joins. There will be follow-ups.Rationale for this change
When a
cross-joinis swapped, the metadata is rebuilt based on the inverted inputs. This can cause a change of metadata when the metadata has conflicting values e.g.:The cross-join should preserve the original metdata when it's swapped.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No