kosiew commented on code in PR #21390:
URL: https://github.com/apache/datafusion/pull/21390#discussion_r3042695391
##########
datafusion/expr/src/expr_schema.rs:
##########
@@ -574,12 +574,21 @@ impl ExprSchemable for Expr {
id: _,
field: Some(field),
}) => Ok(Arc::clone(field).renamed(&schema_name)),
+ Expr::TryCast(TryCast { expr, field }) => expr
Review Comment:
I noticed that `Expr::Cast` and `Expr::TryCast` now have very similar
`to_field()` handling, with the main difference being how nullability is
treated.
Do you think it might be worth pulling this into a small shared helper,
something like "derive cast output field from child field"? It feels like the
kind of logic that could drift again over time if it stays duplicated,
especially if we tweak metadata or nullability behavior later.
##########
datafusion/sqllogictest/test_files/metadata.slt:
##########
@@ -347,5 +369,38 @@ select arrow_metadata(id) from table_with_metadata limit 1;
----
{metadata_key: the id field}
+# Regression test: TRY_CAST should preserve source field metadata
Review Comment:
The new keyed `arrow_metadata(..., 'metadata_key')` cases look great and
cover the regression nicely 👍
As a small follow up, it might be worth adding one case for
`arrow_metadata(TRY_CAST(...))` using the single argument form too. Since that
path returns the full map and this file already exercises both forms for plain
columns, it could help round out the coverage.
--
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]
