[I] Internal error: CAST to an extension-type field over a non-null column, under an aggregate (logical vs physical nullability) [datafusion]

["jayzhan211 (via GitHub)" <gi...@apache.org>]

jayzhan211 opened a new issue, #26469:
URL: https://github.com/apache/datafusion/issues/26469

   ### Describe the bug
   
   When a `CAST` targets an explicit field (one carrying metadata, such as an extension type from a `TypePlanner`), the logical and physical layers compute the output nullability differently:
   
   - The logical `Cast` keeps the input's nullability (`cast_output_field` in `datafusion/expr/src/expr_schema.rs`).
   - The physical `CastExpr::return_field` takes the explicit target field's nullability as-is (`datafusion/physical-expr/src/expressions/cast.rs`).
   
   For a nullable target over a non-null column, the logical field is non-null and the physical one is nullable. The aggregate input schema check rejects that, so any aggregate over the cast fails to plan:
   
   ```
   Internal error: Physical input schema should be the same as the one converted from logical input schema. Differences:
   	- field nullability at index 0 [u]: (physical) true vs (logical) false.
   ```
   
   ### To Reproduce
   
   The sqllogictest UUID type planner (`datafusion/sqllogictest/src/test_context.rs`, used by `cast_extension_type_metadata.slt`) maps `UUID` to a nullable `FixedSizeBinary(16)` field with `ARROW:extension:name = arrow.uuid`:
   
   ```sql
   CREATE TABLE uuid_src AS VALUES
       (arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')),
       (arrow_cast(X'0f0e0d0c0b0a09080706050403020100', 'FixedSizeBinary(16)'));
   
   -- all three fail with the internal error above
   SELECT count(u) FROM (SELECT CAST(column1 AS UUID) AS u FROM uuid_src);
   SELECT count(DISTINCT u) FROM (SELECT CAST(column1 AS UUID) AS u FROM uuid_src);
   SELECT count(*) FROM (SELECT CAST(column1 AS UUID) AS u FROM uuid_src) GROUP BY u;
   ```
   
   These work:
   - the same projection without an aggregate;
   - a nullable input column;
   - `TRY_CAST` (forced nullable on both sides);
   - `SET datafusion.execution.skip_physical_aggregate_schema_check = true`, which returns the correct `2`.
   
   Tested on `main` at 5b5edc7f3c.
   
   ### Expected behavior
   
   `2`, `2`, and two groups of `1`.
   
   ### Additional context
   
   #23169 aligned how the two layers propagate cast metadata, but nullability still follows three rules:
   - logical `cast_output_field` keeps the input's;
   - physical `CastExpr::return_field` uses the explicit target's;
   - physical `CastExpr::nullable` uses `input || target` (#20814).
   
   @paleolimbot noted on #22079 that `nullable()` "frequently disagrees with `return_field().is_nullable()`". This issue is one place that shows up. The other direction is a wrong claim: for an explicit non-null target over a nullable column, `return_field()` reports non-null while the output can contain NULLs.
   
   **Proposed fix.** Use the physical `nullable()` rule in both layers:
   - type-only cast → input nullability;
   - explicit target → `input || target`;
   - `TryCast`, or a physical cast with `safe: true` → nullable.
   
   This keeps the explicit nullable targets that `UnionExec`/`InterleaveExec` and recursive CTEs use to widen nullability. I have this locally as a two-line change plus tests. It fixes the three queries above and passes the full sqllogictest suite and the core integration tests.
   
   **Trade-off.** An explicit non-null target over a nullable input no longer makes `return_field()` report non-null. The one in-tree case is the physical expr adapter, which casts a nullable file column to a non-null table field. Its scan schema is already built from `nullable()`, so batches don't change; only direct readers of `return_field()` see a difference. If narrowing is wanted, @paleolimbot's `NullabilityCast::ForceFalse` idea from #22079 would make it explicit rather than a side effect of the target field.
   
   **Follow-up.** @adriangb's #24833 proposed a single `cast_output_field` shared by both layers, so the rule can't drift again. `datafusion/expr-common/src/casts.rs` already holds cast logic shared between the layers, so it could live there. Would you both be open to that as a second PR after the fix? I'm happy to open the fix PR, or to review if one of you would rather take 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: github-unsubscribe@datafusion.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org