fix: TRY_CAST returns NULL for timestamp/date overflow#22897
fix: TRY_CAST returns NULL for timestamp/date overflow#22897fengys1996 wants to merge 5 commits into
Conversation
Jefffrey
left a comment
There was a problem hiding this comment.
This PR does not add Date array-path coverage yet. For example:
SELECT TRY_CAST(d AS TIMESTAMP(9)) FROM (VALUES (DATE '3000-01-01')) t(d);This depends on the upstream Arrow fix in apache/arrow-rs#9825. Once DataFusion updates to an Arrow version containing that fix, we can add this regression test.
perhaps we can add this right now, but with expected error; that way when the upstream fix is merged and we upgrade to the arrow version we'll automatically know about the fix
| if cast_options.safe { | ||
| if multiplier > 1 && value.checked_mul(multiplier).is_none() { | ||
| return ScalarValue::try_new_null(target_type); | ||
| } | ||
| } else { | ||
| ensure_timestamp_in_bounds(value, multiplier, &source_type, target_type)?; | ||
| } |
There was a problem hiding this comment.
| if cast_options.safe { | |
| if multiplier > 1 && value.checked_mul(multiplier).is_none() { | |
| return ScalarValue::try_new_null(target_type); | |
| } | |
| } else { | |
| ensure_timestamp_in_bounds(value, multiplier, &source_type, target_type)?; | |
| } | |
| match ensure_timestamp_in_bounds(value, multiplier, &source_type, target_type) | |
| { | |
| Ok(()) => {} | |
| Err(_) if cast_options.safe => { | |
| return ScalarValue::try_new_null(target_type); | |
| } | |
| Err(e) => return Err(e), | |
| } |
perhaps like this to reuse the validation logic from ensure_timestamp_in_bounds?
|
Thanks for your review. @Jefffrey
I tried adding the Date array-path SLT coverage by catching the panic as an expected error in 3f4ad71. But the behavior is profile-dependent: debug panics, while release wraps and succeeds. It is not stable for CI, so I reverted it.
Good suggestion, Done in 970d1ae. |
Which issue does this PR close?
Rationale for this change
TRY_CASTshould return NULL on cast failure, but overflowing date/timestamp casts returned errors.What changes are included in this PR?
Are these changes tested?
Yes:
Are there any user-facing changes?
Yes. TRY_CAST for overflowing date/timestamp casts now returns NULL; regular CAST still errors.
Known Limitation
This PR does not add Date array-path coverage yet. For example:
This depends on the upstream Arrow fix in apache/arrow-rs#9825. Once DataFusion updates to an Arrow version containing that fix, we can add this regression test.