Align metadata propagation through Physical and Logical casts - #23169
Align metadata propagation through Physical and Logical casts#23169paleolimbot wants to merge 23 commits into
Conversation
7662d3a to
c74dd77
Compare
| /// Whether to preserve non-extension metadata from the source field. | ||
| /// When true (default), source metadata is merged with target metadata. | ||
| /// When false, only the target field's metadata is used. | ||
| preserve_source_metadata: bool, |
There was a problem hiding this comment.
This is a little gross feeling to me, but there's at least one place in the code that is counting on Cast::return_field() not to execute its child's return_field(): the virtual row number rewrite requires that the cast strips metadata and does not validate its child's Column (which is invalid, at least where it's tested). There may be a better way here.
There was a problem hiding this comment.
Hmm maybe rewrite_file_row_index_expr should not use the CastExpr for this purpose but an "intrinsic" UDF? But I guess this is a different issue then. Not sure how to otherwise avoid that (especially, because we would then require this flag for TryCast if we want feature parity).
There was a problem hiding this comment.
I had an alternate suggestion (compute the target metadata needed in the virtual row number code)
|
@adriangb @tschwarzinger @cyberbeam524 You've all kindly waited for me to finish this...happy to iterate on any of your comments here! |
tschwarzinger
left a comment
There was a problem hiding this comment.
This looks already great to me! Thank you for finding the time to work on this! 🚀
Sorry, if I have many questions as I am not too familiar with the casting system, even after trying to explore it a bit. Feel free to leave them unanswered.
I think, what would be great is a test that checks the consistency between logical and physical planning. I.e., create a few logical expr and a physical expr pairs with the same casting semantics and check whether they come to the same conclusion. Do you think this would be easily possible?
| } | ||
|
|
||
| #[test] | ||
| fn test_try_cast_extension_type_metadata() { |
There was a problem hiding this comment.
Maybe we can combine these two tests and just parameterize them with Expr::TryCast and Expr::Cast ?
Currently, the test for Expr::Cast seems to check that the metadata of the target field is used, while the test for Expr::TryCast does not test that. We can maybe avoid the nullability check in Expr::TryCast as this has nothing to do with extension types and should be done in a separate tests.
However, this could make the test more "complicated" and keeping them apart might be better. Up to you how you want to deal with it.
There was a problem hiding this comment.
I think I did this one!
| use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; | ||
|
|
||
| // Start with source metadata | ||
| let mut metadata = source_field.metadata().clone(); |
There was a problem hiding this comment.
Just to clarify for myself:
- Creating a custom
Expr::Castwith custom metadata (e.g.,my_custom_key) has never really worked in the sense thatmy_custom_keybecomes part of the output metadata. - And it also does not work in this branch as we only consider the extension name / metadata from the target field.
I think this is fine and if we ever need something like this we can track it in a separate ticket with a use case that requires this sort of casts. I just to make sure that we are not removing the ability to include custom metadata for downstream users.
There was a problem hiding this comment.
with custom metadata (e.g., my_custom_key) has never really worked in the sense that my_custom_key becomes part of the output metadata.
Do you mean like adding metadata to the Cast expr itself?
#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
pub struct Cast {
/// The expression being cast
pub expr: Box<Expr>,
/// The `DataType` the expression will yield
pub field: FieldRef,
/// *** New Field: custom metadata **
pub metadata: Metadata,
}We can probably achieve the same thing with a user defined function perhaps
There was a problem hiding this comment.
Alias has been able to do this type of metadata reassignment for some time, and I am not sure it is a good use of the Cast to have it messing with metadata (the confusion that led to the current situation is that the cast operator is being used to manipulate metadata, rather than transform a "data type" in the data-type-or-extension-type sense). I'm happy to tweak this to make it consistent...all I personally need is for the target field extension information not to be dropped (so I can insert an optimizer rule that transforms it into a function call).
There was a problem hiding this comment.
I do personally like the notion that tje field field unambigiously defines the output Field of the expression 🤔
There was a problem hiding this comment.
For arguments sake, and because it seemed a lot cleaner to me, I updated the physical Cast to be explicit about its constituents (i.e., None for metadata passes through from source for backward compatibility, Some(...) forces output, like is required for schema alignment). We were never really casting to a field, we were just using a FieldRef to store three pieces of information.
| cast_options: CastOptions<'static>, | ||
| /// Whether to preserve non-extension metadata from the source field. | ||
| /// When true (default), source metadata is merged with target metadata. | ||
| /// When false, only the target field's metadata is used. |
There was a problem hiding this comment.
If we keep this flag, should we use this flag too in to_field too? Sorry I am not too familiar with the casting system. And if the answer is yes, should we merge the metadata or just retain the source metadata like we currently do (I think)?
There was a problem hiding this comment.
This comes from one specific use that is using the physical cast specifically to strip metadata. I think your later suggestion to remove that usage (and remove this flag) is a good one (but I'll wait for one more opinion on what to do here before I commit).
| /// Whether to preserve non-extension metadata from the source field. | ||
| /// When true (default), source metadata is merged with target metadata. | ||
| /// When false, only the target field's metadata is used. | ||
| preserve_source_metadata: bool, |
There was a problem hiding this comment.
Hmm maybe rewrite_file_row_index_expr should not use the CastExpr for this purpose but an "intrinsic" UDF? But I guess this is a different issue then. Not sure how to otherwise avoid that (especially, because we would then require this flag for TryCast if we want feature parity).
| metadata.remove(EXTENSION_TYPE_METADATA_KEY); | ||
|
|
||
| // Target metadata takes precedence (including extension type metadata) | ||
| for (k, v) in self.target_field.metadata() { |
There was a problem hiding this comment.
Here we use the target metadata fields ("custom metadata") but not in cast_output_field, which confuses me a bit.
There was a problem hiding this comment.
I think I fixed this (and added a test)
| EXTENSION_TYPE_NAME_KEY.to_string(), | ||
| "source.type".to_string(), | ||
| ), | ||
| ("source_key".to_string(), "source_value".to_string()), |
There was a problem hiding this comment.
Maybe adding "target_key" to the target_meta would be beneficial too.
There was a problem hiding this comment.
I think I got this one!
|
|
||
| [dependencies] | ||
| arrow = { workspace = true } | ||
| arrow-schema = { workspace = true } |
There was a problem hiding this comment.
why not just use the re-exported types from arrow (why add a new dependency)?
There was a problem hiding this comment.
I couldn't figure out how to get arrow_schema::extension::<constants> to show up (I don't think schema is pub exported from arrow). I can also inline the constants for metadata key/value.
There was a problem hiding this comment.
This is fine -- probably means we should add some more re-exports to arrow from arrow-schema
alamb
left a comment
There was a problem hiding this comment.
Thanks @paleolimbot and @tschwarzinger -- I can see this is somewhat wacky
I realize the current code takes the metadata from the source field, but that is really confusing to me (as I would expect a cast to produce the output in the target field)
I suspect the challenge is that at some higher levels, when casting to a "data type" that the source metadata can be lost
So what if we made the behavior consistent by:
- the Cast expr itself propagate metadata directly from the
target_field - Have the higher level aps (like
cast_to(data_type)) propagate the source metadata to the target they create
So the semantics of what CastExpr is doing would be clear and consistent
| use arrow_schema::extension::{EXTENSION_TYPE_METADATA_KEY, EXTENSION_TYPE_NAME_KEY}; | ||
|
|
||
| // Start with source metadata | ||
| let mut metadata = source_field.metadata().clone(); |
There was a problem hiding this comment.
with custom metadata (e.g., my_custom_key) has never really worked in the sense that my_custom_key becomes part of the output metadata.
Do you mean like adding metadata to the Cast expr itself?
#[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)]
pub struct Cast {
/// The expression being cast
pub expr: Box<Expr>,
/// The `DataType` the expression will yield
pub field: FieldRef,
/// *** New Field: custom metadata **
pub metadata: Metadata,
}We can probably achieve the same thing with a user defined function perhaps
| /// Derives the output field for a cast expression from the source field. | ||
| /// Derives the output field for a cast expression from the source and target fields. | ||
| /// | ||
| /// Metadata handling: |
There was a problem hiding this comment.
Since this is an internal function, I don't think this documentation will be easy to find -- that is not to say it isn't good to have. However I think we should probably also make a more authoritative docs
For example, it would be nice to link to this detail from here
datafusion/datafusion/expr/src/expr_schema.rs
Line 458 in 9e8dd76
There was a problem hiding this comment.
I didn't quite get there today, but we can add this to the documentation. (edit: done)
| } | ||
| } | ||
|
|
||
| /// Create a new `CastExpr` with an explicit target `FieldRef`, using the |
There was a problem hiding this comment.
can we instead make this a more builder style API -- like
pub fn with_preserve_source_metadata(self, preserve_source_metadata: bool) -> Self {
self.preserve_source_metadata = preserve_source_metadata;
self
}I think that would make th callsites easier to read
let cast= Cast::new(..)
.with_preserve_source_metadata(false)There was a problem hiding this comment.
The physical cast changes let me remove this flag, which I think was kind of ugly anyway. The previous constructors were restored (although maybe we want to make them better).
| let source = Arc::new(Column::new(row_index_name, row_index_idx)); | ||
| let target_field = Arc::new(Field::new("file_row_index", DataType::Int64, true)); | ||
| Ok(Arc::new(CastExpr::new_with_target_field( | ||
| Ok(Arc::new(CastExpr::new_with_exact_target_field( |
There was a problem hiding this comment.
Can you please add a comment here about why it is preservng the source field?
Or is there some way in this code to calculate the desired output metadata here (and attach it to the target field) ?
There was a problem hiding this comment.
The physical cast changes let me remove this function and this change dissappeared
| /// Whether to preserve non-extension metadata from the source field. | ||
| /// When true (default), source metadata is merged with target metadata. | ||
| /// When false, only the target field's metadata is used. | ||
| preserve_source_metadata: bool, |
There was a problem hiding this comment.
I had an alternate suggestion (compute the target metadata needed in the virtual row number code)
| // Start with source metadata | ||
| let mut metadata = source_field.metadata().clone(); | ||
|
|
||
| // Remove any extension type metadata from source - these should not propagate through casts |
There was a problem hiding this comment.
I worry this is not well documented and we may not be consistent across operators
What do you think about adding documentation on what the expectation for metadata / custom types? I feel like if we had a some good refereence we could then evaluate if the code matched the desired behavior. At the moment I don't know what the overall behavior is supposed to be
There was a problem hiding this comment.
We could do this as a separate PR
| if let Some(ext_meta) = target_metadata.get(EXTENSION_TYPE_METADATA_KEY) { | ||
| metadata.insert(EXTENSION_TYPE_METADATA_KEY.to_string(), ext_meta.clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
It feels strange to me that when casting to a new Field that the metadata is taken from the source
I think I would personally expect it to come from the target Field (as we are casting "to" that field) -- though I see that is not what the current code does
There was a problem hiding this comment.
I updated this to make a bit more sense hopefully...the previous behaviour (pass all metadata through because casts are just modifying the DataType) stays, except we strip extension metadata so that ::VARCHAR has an output type of Utf8. If any metadata is specified, it is authoritative.
Unfortunately updating the storage like I did for the physical cast would be a breaking change here (we maybe shouldn't have used a FieldRef 😬 ), so "the previous behaviour" is a bit cryptically detected (mind you, this cryptic detection previously existed in the physical operator...I just moved it here).
|
(I'm missing TryCast tests here, but I updated this a bit to make it a bit more sane, if a little more verbose. Happy to update whatever here, it just seemed like there was an appeteite to make this cleaner than my hacky previous solution) edit: I added TryCast tests + sqllogic tests for the practical usages of this, which is basically casting extension types to and from their storage |
|
@paleolimbot Do you have an update on what's the current status on this PR? It kind of slipped my mind until a staleness notification was issued for #21714 . Anyway I could help moving this forward? |
|
Just let me know when it is ready for the next review and I'll try and get it in |
gene-bordegaray
left a comment
There was a problem hiding this comment.
added comment giving context on my approachcc: @alamb
| let is_type_only = target_field.name().is_empty() | ||
| && target_field.is_nullable() | ||
| && target_field.metadata().is_empty(); | ||
|
|
There was a problem hiding this comment.
I also had this check originally in my PR but changed my apporach because this is fragile and not very explicit for new readers of this code
Say you had this:
// Type only target
Cast::new(expr.clone(), DataType::FixedSizeBinary(16));
// Purpsely empty target
Cast::new_from_field(
expr,
Arc::new(Field::new("", DataType::FixedSizeBinary(16), true)),
);So now both produce a blank, nullable field with empty metadata because it identifies as type-only
The approach here #24725 avoids this but its more invasive because change the pub Cast.field type and a protobuf field
There was a problem hiding this comment.
I agree this is a kludge (borrowed from the Physical cast)...I did this to avoid a breaking change to matches over Expr. The CastTarget in #24725 I think doesn't quite go far enough if we're going to introduce a breaking change here...casts as they currently exist can change the DataType, nullability, and/or metadata. Giving full explicit ness might be:
struct CastTarget {
pub data_type: DataType,
pub nullability: Option<bool>, // None means pass through existing
pub metadata: Option<MetadataTarget>,
}
enum MetadataTarget {
Merge(FieldMetadata),
Replace(FieldMetadata),
}
paleolimbot
left a comment
There was a problem hiding this comment.
Sorry for being slow here (was camping with the kids last week). I'll resolve the merge conflict here in case there's interest in reviving this. I don't personally need more fine grained control over cast targets but am happy if there's interest elsewhere in pursuing it (in SedonaDB we don't use a logical cast to do anything except change a logical type, which for us includes extension types).
| let is_type_only = target_field.name().is_empty() | ||
| && target_field.is_nullable() | ||
| && target_field.metadata().is_empty(); | ||
|
|
There was a problem hiding this comment.
I agree this is a kludge (borrowed from the Physical cast)...I did this to avoid a breaking change to matches over Expr. The CastTarget in #24725 I think doesn't quite go far enough if we're going to introduce a breaking change here...casts as they currently exist can change the DataType, nullability, and/or metadata. Giving full explicit ness might be:
struct CastTarget {
pub data_type: DataType,
pub nullability: Option<bool>, // None means pass through existing
pub metadata: Option<MetadataTarget>,
}
enum MetadataTarget {
Merge(FieldMetadata),
Replace(FieldMetadata),
}|
@paleolimbot what would you think about using this approach for the minor patch since then in the major could do something breaking like #24725 for the major release if we see the need? |
# Conflicts: # datafusion/physical-expr/src/expressions/cast.rs # datafusion/physical-expr/src/expressions/mod.rs # datafusion/physical-expr/src/planner.rs
…-metadata-cleanup
|
Sure! Happy to update as needed. I haven't thought about this in a while and it may not be up to date with recent datafusion changes. |
gene-bordegaray
left a comment
There was a problem hiding this comment.
I think logical protobuf decoding still drops the cast target metadata added during encoding.
to_proto.rs writes field.metadata() but from_proto.rs rebuilds the field using only arrow_type and nullable.
this should be good after ^ and other comments are addressed. Thank you @paleolimbot 🙇
| let source_field = expr.return_field(input_schema)?; | ||
| let has_extension_metadata = source_field | ||
| .metadata() | ||
| .contains_key(EXTENSION_TYPE_NAME_KEY); |
There was a problem hiding this comment.
should this be:
let metadata = source_field.metadata();
let has_extension_metadata = metadata.contains_key(EXTENSION_TYPE_NAME_KEY
|| metadata.contains_key(EXTENSION_TYPE_METADATA_KEY);same with the corresponding line in try_cast
There was a problem hiding this comment.
I don't think so...if there's extension metadata but no extension name, I think it's just metadata without any special handling (or more likely, the producer screwed up). Happy to add if you feel strongly.
| && !contains_type(target_type, &is_struct))) | ||
| { | ||
| let Some(target_field) = retain_field_path(cast.target_field(), &field_path) | ||
| let cast_target_field = cast.target_field(); |
There was a problem hiding this comment.
I think we can use cast_target_field here
|
cc: @alamb |
gene-bordegaray
left a comment
There was a problem hiding this comment.
forgot to correct the breaking change for 55.1, reminded by @timsaucer thank you 🙇
| /// a `DataType`. | ||
| /// See [`CastExpr::new`] for type-only casts where source metadata should | ||
| /// pass through. | ||
| pub fn new_with_target_field( |
There was a problem hiding this comment.
missed this, but since this is going to be in 55.1 patch we cant have the breakin change. Wec ould represent this internally in a way to set up the change in #24725 via something like:
enum CastTarget {
TypeOnly(FieldRef),
Explicit(FieldRef),
}
impl CastTarget {
fn field(&self) -> &FieldRef {
match self {...}
}
fn is_explicit(&self) -> bool { ... }
}There was a problem hiding this comment.
I went with Tim's approach, which was slightly simpler. Introducing a CastTarget is a good idea in general, though.
There was a problem hiding this comment.
My main motivation was to get this backported, which required some simplification.
Restore the two public signatures on `CastExpr` that this branch changed,
so the metadata fixes can be backported to a patch release:
* `CastExpr::new_with_target_field` takes `FieldRef` by value again
* `CastExpr::target_field()` returns `&FieldRef` again
Both are achieved by storing the target `FieldRef` as before and adding a
private `explicit_target` flag that records whether the field was supplied
by the caller or synthesized from a `DataType`. That flag carries exactly
the information the `Option<HashMap<..>>` / `Option<bool>` pair carried, so
`target_metadata()`, `target_nullable()`, `has_explicit_metadata()` and
`has_explicit_nullability()` keep their meaning and are derived from it.
`Hash`/`PartialEq` compare the same components as before, so the field name
still does not participate.
`TryCastExpr` is given the same shape for symmetry. Its metadata-aware API
is new on this branch, so nothing there is a compatibility constraint.
Cast semantics are unchanged: explicit target fields still supply their
metadata and nullability verbatim, type-only casts still pass source
metadata through with the extension type keys stripped, and the output
field name still comes from the source expression.
Comparing the public signatures against the merge base with main, the only
remaining deltas are additions:
+ CastExpr::target_metadata / target_nullable
+ CastExpr::has_explicit_metadata / has_explicit_nullability
+ TryCastExpr::new_with_target_field / target_metadata / target_field
+ expressions::try_cast_with_target_field
`cast_with_target_field` keeps its `&FieldRef` parameter: `mod cast` is
private and it is only re-exported as `pub(crate)`, so it is not part of
the public API.
Verified: `cargo clippy --workspace --all-targets -- -D warnings` clean,
504/504 sqllogictest files pass, and the unit tests for physical-expr,
physical-expr-adapter, physical-plan, pruning, expr, functions and the
datafusion core lib all pass.
Keep cast metadata changes non-breaking
Good call! Should be fixed. I think I got to the comments with Tim's help but happy to update again as well. |
|
Ok, everything is in the green for CI and we no longer have API changes. I'm going to give it one more review and if nobody has objections I'll probably merge in the morning. |
timsaucer
left a comment
There was a problem hiding this comment.
This was a lot of work and a lot of valuable feedback across the community. Thank you @paleolimbot for driving it to completion!
gene-bordegaray
left a comment
There was a problem hiding this comment.
restamp, thanks all @timsaucer @paleolimbot
Which issue does this PR close?
Rationale for this change
The logical
Expr::CastandExpr::TryCasthave aFieldReftarget that was added in #18136 so that logical casts can express a cast to an extension type. In combination with a SQL type planner ( #20676 ) and an optimizer rule, this enabled casting to/from extension types with custom semantics to actually occur. The ability to do this was reverted by #20836 (which removed the original test) and I am not sure that ability ever made it into a release. When investigating this issue, it became clear the logical and physical cast behaviour had diverged with respect to the target field.What changes are included in this PR?
This PR strips specific metadata keys (extension name and extension metadata) when propagating metadata from the source of a cast to the target (because doing so may result in an invalid destination field that consumers could reject), and propagates all metadata from the (logical) cast target field (e.g., so that a cast to an extension type represented by the cast target field will have a
to_field()that communicates the extension type).For the physical cast, this behaviour is replicated exactly (I hope).
Note that actually casting to an extension type can be implemented with an optimizer rule, planner, or by the mechanism I have in the works in #21071 .
Are these changes tested?
Yes
Are there any user-facing changes?
It was in practice not common to create a
Expr::Castwith field metadata internally and thus I don't think users will see metadata changes from the inclusion of metadata from the target field. I would be surprised if stripping the extension name/metadata from the source was disruptive (it was more likely to have caused errors).Superceeds an earlier but similar attempt ( #22162 ).