MaxGekk commented on PR #56205: URL: https://github.com/apache/spark/pull/56205#issuecomment-4592719859
Thanks for the thorough review, @dejankrak-db! I rebased onto master (which now has the SPARK-57033 helpers) and pushed two commits to address the findings: **1 (dup of `instantToTimestampNanos` / `localDateTimeToTimestampNanos`) — done.** Both `stringToTimestampLTZNanos` and `stringToTimestampNTZNanos` now delegate to those helpers; the hand-rolled `epochMicros` re-derivation and `TimestampNanosVal.fromParts` reconstruction are gone. **2 (sub-micro side-channel) — addressed.** Instead of building a micro-only `LocalTime` and reattaching digits 7-9 afterward, I now fold the remainder into a full nano-of-second (`MICROSECONDS.toNanos(segments(6)) + segments(9)`), build a nanosecond `LocalTime`, and let `instantToTimestampNanos` / `localDateTimeToTimestampNanos` extract `instant.getNano % NANOS_PER_MICROS` natively. So the nanos flow *through* the java.time value rather than around it. I did keep `segments(9)` as the transport out of the shared `parseTimestampString`: the alternative (changing `segments(6)` to nanos, or changing the return tuple) would alter the micro path for every existing `TimestampType` / `TIMESTAMP_NTZ` / `TIME` caller, which this PR explicitly avoids. Happy to revisit the side-channel in a dedicated refactor if you'd prefer — let me know. **3 (dup `truncateNanosWithinMicro`) — done.** Deleted; the methods call the shared `truncateNanosWithinMicroToPrecision`. The `[7, 9]` precision guards stay in the parse entry points (the shared helper passes out-of-range precision through), so the `INTERNAL_ERROR` behavior and its test are preserved. **4 (no co-located micro-path regression) — done.** Added a test in `TimestampNanosParseSuite` that calls `stringToTimestamp` / `stringToTimestampWithoutTimeZone` on `.123456789` (9-digit), `.1`, `.000456` (exactly 6), and `.1234567890` (10+) inputs and asserts the microsecond results are unchanged by the segments-array growth and pinned loop bound. **5 (ANSI NTZ hardcodes `allowTimeZone = true`) — done.** Left a `TODO(SPARK-57032)` on `stringToTimestampNTZNanosAnsi` noting the future user-facing cast must choose `allowTimeZone` explicitly rather than relying on the `true` default. All of `TimestampNanosParseSuite`, `DateTimeUtilsSuite`, and `TimestampFormatterSuite` pass, and `./dev/scalastyle` is clean. -- 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]
