sunchao commented on code in PR #24680:
URL: https://github.com/apache/datafusion/pull/24680#discussion_r3867451576
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -184,14 +184,21 @@ pub(crate) struct PushdownChecker<'schema> {
cast_accesses: Vec<CastColumnAccess>,
/// Whether to collect [`Self::cast_accesses`].
collect_cast_accesses: bool,
+ /// Allow field access through a retained Struct cast after schema
adaptation.
Review Comment:
Clarified in
[e79e91236](https://github.com/apache/datafusion/pull/24680/commits/e79e91236e662d3084c7656eae78751027d1633d):
both the source and target must be Struct types. This flag allows
`get_field(CAST(struct_column AS Struct(...)), 'field', ...)` after schema
adaptation; the reader preserves the cast and reads the full source Struct.
Planning keeps the flag disabled so explicit casts retain a residual filter.
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -248,6 +256,56 @@ impl<'schema> PushdownChecker<'schema> {
None
}
+ /// Preserve a Struct cast retained by schema adaptation and read its full
+ /// root. Pruning siblings or moving the cast could change errors or nulls.
+ fn check_cast_struct_field_access(
+ &mut self,
+ func: &ScalarFunctionExpr,
+ ) -> Option<TreeNodeRecursion> {
+ if !self.allow_struct_casts {
+ return None;
+ }
+ let (source, field_names) = func.args().split_first()?;
+ if field_names.is_empty() {
+ return None;
+ }
+ let cast = source.downcast_ref::<CastExpr>()?;
+ let column = cast.expr().downcast_ref::<Column>()?;
+ let index = self.file_schema.index_of(column.name()).ok()?;
+ if !matches!(
+ self.file_schema.field(index).data_type(),
+ DataType::Struct(_)
+ ) {
+ return None;
+ }
+ let return_type = func.return_type();
+ if DataType::is_nested(return_type) &&
!self.is_nested_type_supported(return_type)
+ {
+ return None;
Review Comment:
Thanks, reason logging could help debugging. I'm deferring it to keep this
correctness fix focused. Here, `None` means the retained-Struct-cast special
case did not apply; the caller continues with its normal checks and traversal.
It does not itself reject pushdown or indicate an execution error.
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -309,26 +317,60 @@ fn resolve_field_path<'a>(
}
}
+/// Retain only the selected field path in a cast target, preserving its Struct
+/// ancestors' metadata and nullability. This excludes unselected sibling
+/// conversions while keeping the all-null Struct shortcut for decimal casts.
+fn retain_field_path(field: &FieldRef, path: &[&str]) -> Option<FieldRef> {
+ let Some((name, rest)) = path.split_first() else {
+ return Some(Arc::clone(field));
+ };
+ let DataType::Struct(fields) = field.data_type() else {
+ return None;
+ };
Review Comment:
I kept the empty-path check first because it is the recursion's success
case, including scalar leaves. Swapping the checks returns `None` at a decimal
leaf, so narrowing is abandoned and the whole Struct cast, including unselected
siblings, remains.
I tried the suggested swap:
`test_narrow_decimal_struct_cast_ignores_siblings` failed while converting the
unused `y = "bad"` to `Int32`. The current order preserves the selected leaf
and lets the caller trim the unselected siblings.
--
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]
