jayzhan211 commented on code in PR #26752:
URL: https://github.com/apache/datafusion/pull/26752#discussion_r4412763733
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -85,16 +85,20 @@ pub trait ExprSchemable {
/// output field. Callers that wish to have some mix of source and target
metadata can use
/// Alias or construct an output field themselves (whose metadata will be
used directly).
///
+/// Nullability handling:
Review Comment:
Optional cleanup: the first bullet describes `Expr::nullable` rather than
this function, and the second one repeats it. Maybe just:
```diff
/// Nullability handling:
-/// - The expression is nullable if either the source expression or an
-/// explicit target field is nullable.
-/// - The output field preserves source nullability for a type-only target,
-/// and is nullable if either the source or explicit target is nullable.
+/// - Type-only casts keep the source's nullability.
+/// - Explicit target fields are nullable if either the source or the
target is nullable.
```
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -107,15 +111,23 @@ fn cast_output_field(
target_field.metadata().clone()
};
- let mut f = source_field
- .as_ref()
- .clone()
- .with_data_type(target_field.data_type().clone())
- .with_metadata(metadata);
- if force_nullable {
- f = f.with_nullable(true);
- }
- Arc::new(f)
+ let nullable = if is_type_only {
Review Comment:
Optional cleanup: this can be a single expression, so it reads exactly like
the `Expr::Cast` arm in `nullable()`:
```diff
- let nullable = if is_type_only {
- source_field.is_nullable()
- } else {
- source_field.is_nullable() || target_field.is_nullable()
- };
+ let nullable = source_field.is_nullable()
+ || (!is_type_only && target_field.is_nullable())
+ || force_nullable;
Arc::new(
source_field
.as_ref()
.clone()
.with_data_type(target_field.data_type().clone())
.with_metadata(metadata)
- .with_nullable(nullable || force_nullable),
+ .with_nullable(nullable),
)
```
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -97,3 +97,32 @@ FROM (
);
----
00010203040506070809000102030506 NULL
+
+# Aggregates compare the logical and physical schemas of the cast, so both
+# layers need the same nullability for a nullable explicit target over a
+# required source.
+statement ok
+CREATE TABLE uuid_src AS VALUES
+ (arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')),
+ (arrow_cast(X'0f0e0d0c0b0a09080706050403020100', 'FixedSizeBinary(16)'));
+
+query I
+SELECT count(u) FROM (SELECT CAST(column1 AS UUID) AS u FROM uuid_src);
+----
+2
+
+query I
+SELECT count(DISTINCT u) FROM (SELECT CAST(column1 AS UUID) AS u FROM
uuid_src);
+----
+2
+
+query ?I
+SELECT u, count(*) FROM (SELECT CAST(column1 AS UUID) AS u FROM uuid_src)
+GROUP BY u
+ORDER BY u;
+----
+00010203040506070809000102030506 1
+0f0e0d0c0b0a09080706050403020100 1
Review Comment:
Optional: a type-only cast on top of the UUID cast also failed before this
PR, with the same nullability mismatch on `v`, and passes now. It would be nice
to cover it too:
```sql
query I
SELECT count(v) FROM (SELECT CAST(CAST(column1 AS UUID) AS BYTEA) AS v FROM
uuid_src);
----
2
```
--
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]
