fix: Preserves field metadata when creating logical plan for VALUES expression #17525
Conversation
github-actions
bot
added
logical-expr
Kontinuation
force-pushed
the
fix-issue-17425
branch
from
4de7c9f to
ecb5971
Compare
|
CC @paleolimbot |
Kontinuation
force-pushed
the
fix-issue-17425
branch
from
1b2cda5 to
8eb5b21
Compare
| common_metadata = FieldMetadata::merge_options( | ||
| common_metadata.as_ref(), | ||
| Some(&metadata), | ||
| ); |
There was a problem hiding this comment.
I am not sure this is the exact merge operation we want here, since it will happily merge two completely unrelated extensions (I think) by overwriting the metadata of the first with whatever was encountered last.
It is not perfect, but a potentially safer solution might be to error for any mismatched metadata (i.e., if there's any metadata on these expressions, it has to be identical here or it will error).
There was a problem hiding this comment.
I agree that we should ensure that all the metadata in VALUES must be identical. This has the most unambiguous semantics and does not pose too many restrictions to typical use cases of VALUES. I'll submit commits to fix this later.
There was a problem hiding this comment.
I have updated LogicalPlanBuilder::values to require that all metadata should be identical. I'm not sure if a more permissive approach, such as computing the intersection of metadata, would work, but this more strict rule is a good start anyway.
There was a problem hiding this comment.
Thank you @Kontinuation and @paleolimbot -- this looks good to me.
Kontinuation
force-pushed
the
fix-issue-17425
branch
from
1fa9ae0 to
34fd36f
Compare
|
I merged up to resolve a conflict |
|
And there is now another one! I resolved it and merged up |
|
🚀 |
Which issue does this PR close?
Rationale for this change
The metadata of fields created by
VALUESSQL expression were lost during the creation ofLogicalPlan::Valueslogical plan node. This patch tries to preserve the field metadata.What changes are included in this PR?
This patch takes the metadata of fields in
VALUESexpression into consideration. The schema ofVALUESexpression will contain field with proper metadata.Are these changes tested?
user_defined_scalar_functionstestAre there any user-facing changes?
No public API changes.