Remove the schema checking when creating CrossJoinExec
#4432
Conversation
| let all_columns = { | ||
| let left_schema = left.schema(); | ||
| let right_schema = right.schema(); | ||
| let left_fields = left_schema.fields().iter(); | ||
| let right_fields = right_schema.fields().iter(); | ||
| left_fields.chain(right_fields).cloned().collect() | ||
| }; | ||
|
|
||
| let schema = Arc::new(Schema::new(all_columns)); |
There was a problem hiding this comment.
Could you use Schema::merge here instead?
There was a problem hiding this comment.
You could do something like this. This would preserve schema metadata as well.
let input_schemas = vec![left.schema().as_ref().clone(), right.schema().as_ref().clone()];
let schema = Arc::new(Schema::try_merge(input_schemas)?);There was a problem hiding this comment.
Sorry, I'm afraid try_merge doesn't fit this context.
What we want here is to concatenate schema, but not merge schema. It could happen that a table cross joins with itself, or two tables have same named columns, in which cases, merge doesn't work.
There was a problem hiding this comment.
For the metadata, we should also do concatenation, instead of merging.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This PR makes sense to me -- thank you @HaoYang670
|
As I think all comments have been addressed, I am merging it in |
3 participants
Signed-off-by: remzi 13716567376yh@gmail.com
Which issue does this PR close?
Closes #4431 .
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?
Yes, rename
CrossJoinExec::try_newtoCrossJoinExec::newand update the return type toSelf.