[SPARK-57458][SQL][FOLLOWUP] Avoid non-local return in XmlInferSchema.tryParseTimestampNTZ#56916
[SPARK-57458][SQL][FOLLOWUP] Avoid non-local return in XmlInferSchema.tryParseTimestampNTZ#56916cloud-fan wants to merge 1 commit into
Conversation
|
cc @MaxGekk |
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 1 nit.
Clean, behavior-identical cleanup. filter(p).foreach runs iff Some && p — exactly Option.exists(p) — and the && short-circuit keeps the parse from running when the flag is off, as before; both the old NonLocalReturnControl (a ControlThrowable, not an Exception) and the new plain return escape the surrounding catch { case _: Exception } uncaught, so the result is unchanged (and slightly more robust). One nit inline.
Findings
Nits (1)
- The relocated
// ... more than 6 digitscomment is imprecise vs thenanosWithinMicro != 0check — see inline.
| nanosOpt.filter(_.nanosWithinMicro != 0).foreach { _ => | ||
| return Some(TimestampNTZNanosType(9)) | ||
| } | ||
| // Prefer nanosecond type when the fractional seconds part has more than 6 digits, |
There was a problem hiding this comment.
Nit (pre-existing wording, but you're moving this comment so it's a natural moment): "more than 6 digits" doesn't quite match the nanosWithinMicro != 0 test below it. A 7-digit fraction with a trailing zero — e.g. .1234560 → 123456000 ns → nanosWithinMicro == 0 — has more than 6 digits yet stays TimestampNTZType; the actual trigger is a nonzero sub-microsecond remainder, not the digit count. Maybe: // Prefer nanosecond type when there is a nonzero sub-microsecond component (nanosWithinMicro != 0) that TimestampNTZType cannot represent.
The refactor itself is exactly equivalent — nice cleanup.
….tryParseTimestampNTZ ### What changes were proposed in this pull request? This is a followup of SPARK-57458. The nanosecond-precision timestamp inference in `XmlInferSchema.tryParseTimestampNTZ` used a `return` statement inside an `Option.foreach` closure, which is a non-local return implemented via a control-flow exception. This replaces it with a plain boolean check plus a local `return`, keeping the behavior identical while avoiding the non-local return. ### Why are the changes needed? Non-local returns rely on throwing/catching a `NonLocalReturnControl` exception, which is discouraged and flagged by lint tooling. The rewrite is equivalent and clearer. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing tests for nanosecond timestamp inference in the XML datasource cover this code path. ### Was this patch authored or co-authored using generative AI tooling? Yes, this patch was co-authored using generative AI tooling.
cloud-fan
force-pushed
the
cloud-fan/xml-nano-nonlocal-return
branch
from
676e18a to
134a178
Compare
2 participants
What changes were proposed in this pull request?
This is a followup of SPARK-57458. The nanosecond-precision timestamp inference in
XmlInferSchema.tryParseTimestampNTZused areturnstatement inside anOption.foreachclosure, which is a non-local return implemented via a control-flow exception. This replaces it with a plain boolean check plus a localreturn, keeping the behavior identical while avoiding the non-local return.Why are the changes needed?
Non-local returns rely on throwing/catching a
NonLocalReturnControlexception, which is discouraged and flagged by lint tooling. The rewrite is equivalent and clearer.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests for nanosecond timestamp inference in the XML datasource cover this code path.
Was this patch authored or co-authored using generative AI tooling?
Yes, this patch was co-authored using generative AI tooling.