fix: preserve source field metadata in TryCast expressions#21390
fix: preserve source field metadata in TryCast expressions#21390adriangb wants to merge 4 commits intoapache:mainfrom
Conversation
TryCast's `to_field` implementation fell through to a default case that created a new Field without metadata, unlike Cast which had a dedicated handler preserving the source field's metadata. This adds the same metadata-preserving logic for TryCast. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
bot
added
logical-expr
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that metadata is preserved even when TRY_CAST returns NULL due to an invalid conversion (e.g. non-numeric string to INT). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| id: _, | ||
| field: Some(field), | ||
| }) => Ok(Arc::clone(field).renamed(&schema_name)), | ||
| Expr::TryCast(TryCast { expr, field }) => expr |
There was a problem hiding this 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.
| ---- | ||
| {metadata_key: the id field} | ||
|
|
||
| # Regression test: TRY_CAST should preserve source field metadata |
There was a problem hiding this 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.
3 participants
Which issue does this PR close?
N/A — discovered while investigating metadata propagation through cast expressions in #21322
Rationale for this change
Expr::Castpreserves the source field's metadata through a dedicatedto_fieldhandler inexpr_schema.rs, butExpr::TryCastfell through to the default case which creates aField::new(...)without any metadata. This caused source column metadata to be silently dropped when usingTRY_CAST.What changes are included in this PR?
to_fieldhandler forExpr::TryCastinexpr_schema.rsthat preserves source field metadata (matchingExpr::Castbehavior), while keeping TryCast's always-nullable semantics.metadata.sltverifying metadata preservation throughTRY_CASTon both timestamp and integer columns.Are these changes tested?
Yes — new sqllogictest cases in
metadata.sltusingarrow_metadata()to verify metadata is preserved throughTRY_CAST.Are there any user-facing changes?
TRY_CASTnow preserves source field metadata, consistent withCASTbehavior.🤖 Generated with Claude Code