gene-bordegaray commented on code in PR #23169:
URL: https://github.com/apache/datafusion/pull/23169#discussion_r3897693593
##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -69,63 +78,78 @@ pub struct CastExpr {
impl PartialEq for CastExpr {
fn eq(&self, other: &Self) -> bool {
self.expr.eq(&other.expr)
- && self.target_field.eq(&other.target_field)
+ && self.target_type.eq(&other.target_type)
+ && self.target_metadata.eq(&other.target_metadata)
+ && self.target_nullable.eq(&other.target_nullable)
&& self.cast_options.eq(&other.cast_options)
}
}
impl Hash for CastExpr {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.expr.hash(state);
- self.target_field.hash(state);
+ self.target_type.hash(state);
+ // Hash the metadata by iterating over sorted keys for deterministic
ordering
+ if let Some(metadata) = &self.target_metadata {
+ let mut entries: Vec<_> = metadata.iter().collect();
+ entries.sort_by_key(|(k, _)| *k);
+ for (k, v) in entries {
+ k.hash(state);
+ v.hash(state);
+ }
+ }
+ self.target_nullable.hash(state);
self.cast_options.hash(state);
}
}
impl CastExpr {
/// Create a new `CastExpr` using only a `DataType`.
///
- /// This constructor is provided for compatibility with existing call sites
- /// that only know the target type. It synthesizes a ``Field`` with the
- /// given type (**nullable by default**) and no name metadata. Callers
that
- /// already have a `FieldRef` (for example, coming from schema inference
or a
- /// resolved column) should prefer [`CastExpr::new_with_target_field`],
which
- /// preserves the field's name, nullability, and other metadata. In other
- /// words:
+ /// This constructor creates a type-only cast where metadata and
nullability
+ /// are passed through from the source expression (with extension type keys
+ /// stripped from metadata). This is the most common use case when you only
+ /// need to change the data type.
///
- /// * use `new()` when only a `DataType` is available and you want the
legacy
- /// semantics of a type-only cast
- /// * use `new_with_target_field()` when you need explicit field
- /// metadata/name/nullability preserved
+ /// For explicit control over the output field's metadata and nullability,
+ /// use [`CastExpr::new_with_target_field`] or the individual builder
methods.
pub fn new(
expr: Arc<dyn PhysicalExpr>,
cast_type: DataType,
cast_options: Option<CastOptions<'static>>,
) -> Self {
- Self::new_with_target_field(
+ Self {
expr,
- cast_type.into_nullable_field_ref(),
- cast_options,
- )
+ target_type: cast_type,
+ target_metadata: None,
+ target_nullable: None,
+ cast_options: cast_options.unwrap_or(DEFAULT_CAST_OPTIONS),
+ }
}
/// Create a new `CastExpr` with an explicit target `FieldRef`.
///
- /// The provided `target_field` is used verbatim for the expression's
- /// return schema, so the field's name, nullability, and other metadata are
- /// preserved. This is the preferred constructor when the caller already
- /// has field information (for example, during logical-to-physical
planning).
+ /// The provided `target_field` determines the output characteristics:
+ /// - The field's data type becomes the cast target type
+ /// - The field's metadata is used exactly as provided
+ /// - The field's nullability is preserved
+ ///
+ /// This is the preferred constructor when the caller has explicit field
+ /// information that should be used exactly (for example, during schema
+ /// enforcement or adapter layers).
///
- /// See [`CastExpr::new`] for the compatibility constructor that only
accepts
- /// a `DataType`.
+ /// See [`CastExpr::new`] for type-only casts where source metadata should
+ /// pass through.
pub fn new_with_target_field(
Review 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:
```rust
enum CastTarget {
TypeOnly(FieldRef),
Explicit(FieldRef),
}
impl CastTarget {
fn field(&self) -> &FieldRef {
match self {...}
}
fn is_explicit(&self) -> bool { ... }
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
