gene-bordegaray commented on code in PR #24670:
URL: https://github.com/apache/datafusion/pull/24670#discussion_r3903228823
##########
datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:
##########
@@ -45,5 +45,55 @@ FROM (
----
00010203040506070809000102030506 arrow.uuid
-statement error DataFusion error: Optimizer rule 'simplify_expressions'
failed[\s\S]*TryCast from FixedSizeBinary\(16\) to
FixedSizeBinary\(16\)<\{"ARROW:extension:name": "arrow\.uuid"\}> is not
supported
-SELECT TRY_CAST(arrow_cast(X'00010203040506070809000102030506',
'FixedSizeBinary(16)') AS UUID);
+# TRY_CAST to extension type should also preserve extension metadata
+query ?T
+SELECT
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ arrow_metadata(
+ TRY_CAST(
+ arrow_cast(X'00010203040506070809000102030506',
'FixedSizeBinary(16)')
+ AS UUID
+ ),
+ 'ARROW:extension:name'
+ );
+----
+00010203040506070809000102030506 arrow.uuid
+
+# TRY_CAST to UUID from a subquery
+query ?T
+SELECT
+ TRY_CAST(raw AS UUID),
+ arrow_metadata(TRY_CAST(raw AS UUID), 'ARROW:extension:name')
+FROM (
+ VALUES (
+ arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
+ )
+) AS uuids(raw);
+----
+00010203040506070809000102030506 arrow.uuid
+
+# arrow_cast from UUID to same underlying type (FixedSizeBinary(16)) strips
+# extension metadata (type-only cast semantics)
+query ?T
+SELECT
+ arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
+ arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
'ARROW:extension:name')
+FROM (
+ SELECT CAST(arrow_cast(X'00010203040506070809000102030506',
'FixedSizeBinary(16)') AS UUID) AS uuid_val
+);
+----
+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast
semantics)
+query ?T
Review Comment:
Yes, it is for a different bug but when i rooginally made the minimal fix
(my top commit now on this brnach) it fails some sqllogictests because of
#23169 not being merged. Specifically:
```sql
SELECT CAST(raw AS UUID), arrow_metadata(CAST(raw AS UUID),
'ARROW:extension:name');
```
Expected on main:
```text
00010203040506070809000102030506 arrow.uuid
```
But with the projection fix alone the projection starts to actually
preserver logical schema then ignores the UUID target metadata, so the result:
```diff
- arrow.uuid
+ NULL
```
#24831 handles this with this check:
```rust
let metadata = if target_field.metadata().is_empty() {
source_field.metadata().clone()
} else {
target_field.metadata().clone()
};
```
as prposed in your brnach #24831 but it has the issue that I talk about
[here](https://github.com/apache/datafusion/pull/24670#discussion_r3865163140)
which is hwy I opted into stacking on #23169 to handle that case while not
allowing another edge case to creep in.
So this is getting a bit tricky to handle. That particular test is from
another PR, but solving just the bug at at the surface level unveils more
underlying issues with casting and metadata that this relies on. I would think
that getting this in the minor patch with #23169 would be the good short term
solution, Then I read [your
comment](https://github.com/apache/datafusion/pull/23169#issuecomment-5489686098)
and I think this could be a viable breaking change after some more discussion
regarding how we want these semantics to behave.
Thanks for taking time to investigate all this @adriangb 🙇
--
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]
