zhuqi-lucas commented on code in PR #24125:
URL: https://github.com/apache/datafusion/pull/24125#discussion_r3730087936
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -273,6 +273,35 @@ struct DefaultPhysicalExprAdapterRewriter {
physical_file_schema: SchemaRef,
}
+/// Outcome of walking a `get_field` key path through nested struct fields.
+enum FieldPathResolution<'a> {
+ /// The leaf field the path points at.
+ Found(&'a FieldRef),
+ /// Some key along the path does not exist, so the access reads as null.
+ Missing,
+ /// An intermediate field is not a struct, so the path cannot be resolved
+ /// statically.
+ NotAStruct,
+}
+
+/// Follow a `get_field` key path (`['a', 'b']` for `s['a']['b']`) through
+/// nested struct fields.
+fn resolve_field_path<'a>(fields: &'a Fields, path: &[&str]) ->
FieldPathResolution<'a> {
+ let Some((field_name, rest)) = path.split_first() else {
+ return FieldPathResolution::NotAStruct;
Review Comment:
Minor: an empty `path` here returns `NotAStruct`, which reads a little oddly
— an empty path is not really "not a struct". It is unreachable given the
non-empty `field_name_exprs` guard in `try_narrow_struct_cast`, so a one-line
comment noting it is a defensive default would save the next reader a
double-take.
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -282,13 +311,130 @@ impl DefaultPhysicalExprAdapterRewriter {
return Ok(Transformed::yes(transformed));
}
+ if let Some(transformed) = self.try_narrow_struct_cast(&expr)? {
+ return Ok(Transformed::yes(transformed));
+ }
+
if let Some(column) = expr.downcast_ref::<Column>() {
return self.rewrite_column(Arc::clone(&expr), column);
}
Ok(Transformed::no(expr))
}
+ /// Rewrite `get_field(cast(s AS Struct<..>), 'f')` into
Review Comment:
Nice fix. One design question for the record: did you consider teaching the
pushdown side (`PushdownChecker` / row-filter builder) to see through the cast
— i.e. recognize `get_field(cast(col), 'f')` — instead of narrowing it here?
I assume narrowing was chosen because (a) it avoids materializing the whole
cast struct just to read one field, and (b) it fixes it at the source, so
*every* consumer that pattern-matches `get_field(column, 'f')` benefits — not
just the row filter — rather than loosening the pushdown contract to see
through arbitrary casts. Worth capturing that rationale.
Relatedly, the PR notes the broader planning-vs-runtime schema divergence is
intentionally out of scope — a tracking issue for the "safe by construction"
mechanism (e.g. post-decode filtering in `ParquetOpener`) would be good so it
is not lost. Happy to file it.
--
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]
