jayzhan211 commented on PR #25013:
URL: https://github.com/apache/datafusion/pull/25013#issuecomment-5642410403
@peterxcli , a suggestion:
**`try_narrow_struct_cast` can now fail the query instead of skipping the
rewrite**
`schema_rewriter.rs:565` rebuilds the accessor against the physical file
schema:
```rust
let extracted = Arc::new(ScalarFunctionExpr::try_new(
Arc::new(get_field_expr.fun().clone()),
args,
&self.physical_file_schema,
Arc::new(get_field_expr.config_options().clone()),
)?) as Arc<dyn PhysicalExpr>;
```
`try_new` runs `fields_with_udf` and `return_field_from_args` on the
**physical** types — which differ from the logical ones by construction, since
that difference is why the adapter inserted the cast you're about to drop. With
the old `try_downcast_func::<GetFieldFunc>` gate the `?` was safe
(`Signature::user_defined`, permissive). It isn't safe for arbitrary UDFs: a
function that declares `struct_field_access` but doesn't accept the physical
types now aborts the whole query, where every other path in this rewriter
degrades to `Ok(None)`.
Reproduced on this branch using your own `FieldAt` helper with an `Exact`
signature pinned to the logical struct type. File `s: Struct<value: Int32>`,
registered schema `s: Struct<value: Int64>`, query `SELECT id FROM t WHERE
field_at('value', s) > 5`:
```
declare_access=false: Ok([...]) // 1 row, as expected
declare_access=true: Err(Plan("Failed to coerce arguments to satisfy a call
to
'field_at' function: coercion from Utf8, Struct(\"value\": Int32) to the
signature Exact(Utf8, Struct(\"value\": Int64)) failed"))
```
Declaring the capability should never make a working query fail. Make the
rebuild best-effort:
```diff
- let extracted = Arc::new(ScalarFunctionExpr::try_new(
- Arc::new(get_field_expr.fun().clone()),
- args,
- &self.physical_file_schema,
- Arc::new(get_field_expr.config_options().clone()),
- )?) as Arc<dyn PhysicalExpr>;
+ // A third-party accessor may not accept the physical field types.
That
+ // is a missed rewrite, not a query error: keep the cast and bail
out.
+ let Ok(extracted) = ScalarFunctionExpr::try_new(
+ Arc::new(get_field_expr.fun().clone()),
+ args,
+ &self.physical_file_schema,
+ Arc::new(get_field_expr.config_options().clone()),
+ ) else {
+ return Ok(None);
+ };
+ let extracted = Arc::new(extracted) as Arc<dyn PhysicalExpr>;
```
Two follow-ups worth folding in:
1. `FieldAt::signature` is currently `Signature::any(2,
Volatility::Immutable)` at all three construction sites, i.e. the one shape
that can't hit this. Point one of them at an `Exact` signature over the logical
struct type and assert the evolved query returns the right rows (unpruned)
rather than erroring.
2. The `struct_field_access` doc promises implementors only "reordered or
narrowed source structs". This rewrite does more — it drops the type-adapting
cast and re-applies the conversion to the output, so the UDF sees the file's
physical field types. Please say so in the contract, since that is the
assumption this code actually relies on.
--
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]
