Fix more instances of schema missing metadata #13068
Conversation
github-actions
bot
added
logical-expr
There was a problem hiding this comment.
Thanks @itsjunetime -- I agree these all looks like good fixes. I am a little worried about the lack of test coverage but I agree most of the situations look like they would be hard to write tests for.
I had some thoughts in apache/arrow-rs#6576 about how to make the API for building Schema harder to avoid missing metadata -- maybe we can use this PR as an use case for that API
| @@ -1014,14 +1014,21 @@ impl DefaultPhysicalPlanner { | |||
| }) | |||
| .collect(); | |||
|
|
|||
| let metadata: HashMap<_, _> = left_df_schema | |||
There was a problem hiding this comment.
this one we should be able to write a test for -- perhaps with a query with a JOIN in https://github.com/apache/datafusion/blob/main/datafusion/sqllogictest/test_files/metadata.slt
|
It would be great to figure out how to test this code better but maybe that is not feasible at this time -- thanks again @itsjunetime and @jayzhan211 |
Which issue does this PR close?
This will not close, but rather just helps with the situation in, #12733.
Rationale for this change
As I was trying to fix other issues, I ran into more instances where Schema metadata could theoretically be dropped. These specific issues don't have reproducers (since I just found them while working on other stuff), but I think they are clearly issues nonetheless.
What changes are included in this PR?
Mostly changing
Schema::newtoSchema::new_with_metadataandschema_builder.finish()toschema_builder.finish().with_metadata(...)Are these changes tested?
Yes, all tests still pass.
Are there any user-facing changes?
No