viirya commented on code in PR #24680:
URL: https://github.com/apache/datafusion/pull/24680#discussion_r3864814890
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -309,26 +313,58 @@ fn resolve_field_path<'a>(
}
}
+/// Retain a field path without changing its ancestors' metadata or
nullability.
+fn retain_field_path(field: &FieldRef, path: &[&str]) -> Option<FieldRef> {
Review Comment:
`retain_field_path` and the existing `resolve_field_path` sit next to each
other and read as near-synonyms, but they do different jobs: one resolves a key
path to a leaf for inspection, the other rebuilds a trimmed cast target that
keeps the ancestors intact. The doc comment explains the *what* ("retain a
field path without changing its ancestors' metadata or nullability"); a clause
on *why it exists* — to exclude unselected siblings from a conversion while
keeping the Struct ancestors for Arrow's all-null shortcut — would connect it
to the decimal case that motivates it. A name closer to that purpose
(`trim_cast_target_to_path`, say) would also help, though renaming is entirely
your call.
##########
datafusion/physical-expr-adapter/src/schema_rewriter.rs:
##########
@@ -271,6 +272,9 @@ impl PhysicalExprAdapter for DefaultPhysicalExprAdapter {
struct DefaultPhysicalExprAdapterRewriter {
logical_file_schema: SchemaRef,
physical_file_schema: SchemaRef,
+ // Retain generated casts so their pointer identity remains reliable even
+ // after a wider cast has been removed from the expression tree.
+ generated_struct_casts: HashMap<*const (), Arc<dyn PhysicalExpr>>,
Review Comment:
This map's soundness rests on three things, and only the second is currently
stated:
1. `rewrite()` uses `expr.transform(...)`, which is bottom-up, so a `Column`
becomes a generated cast *before* the parent `get_field` is visited — that is
what makes the child `Arc` the parent sees the same allocation that was
recorded.
2. The map holds an owned `Arc` clone, so the allocation cannot be freed and
its address recycled (the part the current comment covers).
3. The rewriter is constructed per `rewrite()` call, so keys never leak
across expressions.
Point 1 is the fragile one: switching to `transform_down`, or hoisting the
rewriter to be reused across expressions, would silently stop narrowing. The
failure is fail-closed — it would under-optimise rather than mis-narrow — so
this isn't a correctness worry, but it is a silent performance regression that
no test would catch. Could the comment name the traversal-order dependency
explicitly?
Separately, and only if it isn't invasive: was a structural marker
considered instead of pointer identity — threading "this cast was generated"
out through the rewrite result, or a thin wrapper type around generated casts?
That would remove this whole class of concern rather than documenting around
it. If you tried it and it spread too far through the rewriter, saying so in
the comment would save the next person from re-litigating 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]
