dejankrak-db commented on PR #56205:
URL: https://github.com/apache/spark/pull/56205#issuecomment-4588433174
### Architecture / Simplicity
**1. (High) New parse APIs duplicate the existing
`instantToTimestampNanos` / `localDateTimeToTimestampNanos` path.**
`sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala`
(`stringToTimestampLTZNanos`/`stringToTimestampNTZNanos`). Master already
has these helpers (from the preceding SPARK-57033 work) that take a `java.time`
value and produce the `(epochMicros, nanosWithinMicro)` pair with precision
truncation; the new LTZ body is a near-verbatim copy of
`stringToTimestamp` plus one line, manually re-deriving `epochMicros`. Carry
nanos in the `java.time` value (see #2) and delegate to the existing helpers,
deleting the hand-rolled
reconstruction.
**2. (High) Sub-microsecond precision is threaded through an output-only
`segments(9)` side-channel rather than carried in the parsed
`LocalTime`/`Instant`.** `SparkDateTimeUtils.scala` (array growth + slot-9
population; consumed in
the new APIs). The code builds `LocalTime` with only micros and smuggles
digits 7-9 around the `java.time` value, reattaching afterward — yet
`LocalTime`/`Instant` support nanos natively and `instantToTimestampNanos`
already extracts
`instant.getNano % NANOS_PER_MICROS`. This side-channel is the root cause
of #1 and forces the fragile "slot 9 written outside the loop, slots 7-8
written-but-never-read" invariant. Retain full fractional nanos, construct a
nanosecond
`LocalTime`, and delegate.
**3. (Medium) New private `truncateNanosWithinMicro` duplicates the
existing `truncateNanosWithinMicroToPrecision`.** `SparkDateTimeUtils.scala`.
Same arithmetic; differs only by throwing on out-of-range (a dead arm — callers
already
validate `p ∈ [7,9]`) and returning `Short`. Delete it and call
`truncateNanosWithinMicroToPrecision(...).toShort`, or remove entirely via
#1/#2.
### Low-level
**4. (Medium) No test pins the unchanged micro path through the edited
shared `parseTimestampString`.**
`sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampNanosParseSuite.scala`.
The highest-blast-radius change
(loop bound + segment array) affects every
`TimestampType`/`TIMESTAMP_NTZ`/`TIME` cast, but the new suite exercises only
the new nanos APIs. `DateTimeUtilsSuite` already covers the >6-digit
micro-truncation case in a sibling suite (so
not blocking), but the central back-compat claim deserves a co-located
regression assertion calling
`stringToTimestamp`/`stringToTimestampWithoutTimeZone` on `.123456789`-style
inputs.
**5. (Low) ANSI NTZ variant hardcodes `allowTimeZone = true` (silently
discards a zone), with no strict-reject ANSI path.** `SparkDateTimeUtils.scala`
(`stringToTimestampNTZNanosAnsi`). Asymmetric vs. existing strict NTZ casts, but
documented in scaladoc and tested, and the APIs aren't wired to
user-facing casts yet — acceptable for now. Leave a `// TODO(SPARK-57032
wiring)` noting the ANSI cast must decide `allowTimeZone` explicitly when wired.
--
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]
