You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2026/06/03 09:14:07 UTC
[PR] [WIP][SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
MaxGekk opened a new pull request, #56295:
URL: https://github.com/apache/spark/pull/56295
### What changes were proposed in this pull request?
Extend the `TimestampFormatter` family with additive, nanosecond-aware parse and format methods that produce and consume `org.apache.spark.unsafe.types.TimestampNanosVal` (`epochMicros: Long` + `nanosWithinMicro: Short` in `[0, 999]`) at a target fractional precision `p` in `[7, 9]`:
- New trait methods: `parseNanos` / `parseNanosOptional` (LTZ), `parseWithoutTimeZoneNanos` / `parseWithoutTimeZoneNanosOptional` (NTZ, plus a `final` `allowTimeZone = true` overload), and `formatNanos`.
- `Iso8601TimestampFormatter`: `extractNanos` / `extractNanosNTZ` build the `Instant` / `LocalDateTime` and delegate to `SparkDateTimeUtils.instantToTimestampNanos` / `localDateTimeToTimestampNanos`; `formatNanos` floors sub-`precision` digits and renders the reconstructed instant.
- `DefaultTimestampFormatter`: delegates to the SPARK-57032 nanos entry points.
- `LegacyFastTimestampFormatter` / `LegacySimpleTimestampFormatter`: explicitly reject nanosecond precision under the `LEGACY` time parser policy (they cap at micro resolution).
Sub-precision fractional digits are truncated (floored), consistent with SPARK-57032. All existing microsecond methods are unchanged (additive API).
This is marked `[WIP]` while the downstream datasource wiring (separate sub-tasks) is in progress.
### Why are the changes needed?
Today `TimestampFormatter` is microsecond-only and discards the 7th-9th fractional digits. The JSON and CSV datasources drive all timestamp parsing/formatting through `TimestampFormatter`, so they cannot round-trip 7-9 digit fractions until the formatter is nanos-aware. This is the foundational unblocker for nanosecond support in those datasources (parent: SPARK-56822).
### Does this PR introduce _any_ user-facing change?
No. The new formatter API is additive and gated for use behind `spark.sql.timestampNanosTypes.enabled` by its callers.
### How was this patch tested?
New cases in `TimestampFormatterSuite`: parse/format round-trip for `p` in `[7, 9]` across ISO default and custom patterns (LTZ and NTZ); boundary values (`nanosWithinMicro` 0 and 999, pre-epoch instants, the 0001/1582/1970/9999 corpus); truncation rule; NTZ time-zone rejection; and LEGACY-mode rejection.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor (Claude Opus 4.8)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3349131537
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -38,15 +38,14 @@ import org.apache.spark.sql.errors.ExecutionErrors
import org.apache.spark.sql.internal.LegacyBehaviorPolicy._
import org.apache.spark.sql.internal.SqlApiConf
import org.apache.spark.sql.types.{Decimal, TimestampNTZType}
-import org.apache.spark.unsafe.types.UTF8String
+import org.apache.spark.unsafe.types.{TimestampNanosVal, UTF8String}
sealed trait TimestampFormatter extends Serializable {
/**
* Parses a timestamp in a string and converts it to microseconds.
*
- * @param s
- * \- string with timestamp to parse
+ * @param s String with timestamp to parse
Review Comment:
Good point - I reverted the `@param` reflow so the pre-existing methods are left untouched and the diff stays focused on the feature. The file is back to its original dashed multi-line `@param` style.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56295:
URL: https://github.com/apache/spark/pull/56295#issuecomment-4613814717
@uros-b Could you review this PR, please. We need this to support casting timestamps to nanos, in literals, text datasource, hive results, in some expressions and so on.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3350553954
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -158,6 +158,109 @@ sealed trait TimestampFormatter extends Serializable {
// did not fail if timestamp contained zone-id or zone-offset component and instead ignored it.
parseWithoutTimeZone(s, true)
+ /**
+ * Parses a timestamp in a string and converts it to a [[TimestampNanosVal]] (epoch microseconds
+ * plus a sub-microsecond remainder in `[0, 999]`) for `TIMESTAMP_LTZ(precision)`. Fractional
+ * digits beyond `precision` are truncated (floored), matching the cast/parse rule used by the
+ * microsecond path and `SparkDateTimeUtils`.
+ *
+ * @param s
+ * \- string with timestamp to parse
+ * @param precision
+ * \- the target fractional-second precision in `[7, 9]`
+ * @return
+ * the parsed value as a [[TimestampNanosVal]].
+ */
+ @throws(classOf[ParseException])
+ @throws(classOf[DateTimeParseException])
+ @throws(classOf[DateTimeException])
+ def parseNanos(s: String, precision: Int): TimestampNanosVal
+
+ /**
+ * Optional counterpart of [[parseNanos]]. The result is `None` on invalid input.
+ */
+ def parseNanosOptional(s: String, precision: Int): Option[TimestampNanosVal] =
+ try {
+ Some(parseNanos(s, precision))
+ } catch {
+ case _: Exception => None
+ }
Review Comment:
This will swallow the user-facing error on legacy formatters. Do we really want silent nulls instead of errors in such scenarios?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #56295: [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal
URL: https://github.com/apache/spark/pull/56295
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3349251291
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -227,6 +308,42 @@ class Iso8601TimestampFormatter(
} catch checkParsedDiff(s, legacyFormatter.parse)
}
+ // `checkParsedDiff` only uses the legacy parse to decide whether to raise an upgrade exception
+ // and never returns its result, so the legacy formatter (microsecond-only) is fine here even on
+ // the nanos path. The returned `TimestampNanosVal.ZERO` is discarded.
+ protected def legacyNanosParse(str: String): TimestampNanosVal = {
+ legacyFormatter.parse(str)
+ TimestampNanosVal.ZERO
+ }
+
+ override def parseNanosOptional(s: String, precision: Int): Option[TimestampNanosVal] = {
+ try {
+ val parsePosition = new ParsePosition(0)
+ val parsed = formatter.parseUnresolved(s, parsePosition)
+ if (parsed != null && s.length == parsePosition.getIndex) {
+ Some(extractNanos(parsed, precision))
+ } else {
+ None
+ }
+ } catch {
+ case NonFatal(_) => None
+ }
+ }
+
+ private def extractNanos(parsed: TemporalAccessor, precision: Int): TimestampNanosVal = {
Review Comment:
Good point - the contract was enforced inconsistently. The `DefaultTimestampFormatter` parse path rejects out-of-range precision (the `stringToTimestamp*Nanos` helpers throw), but the `Iso8601` path went through `instantToTimestampNanos` / `localDateTimeToTimestampNanos`, whose truncation silently passed any `p` through.
I added the guard at the shared choke point: `truncateNanosWithinMicroToPrecision` now raises `SparkException.internalError` for `p` outside `[7, 9]`. Both converters funnel through it, so both the NTZ and LTZ paths - in parse and format directions - now enforce the bound uniformly. I used the internal-error mechanism (matching the existing string-helper guards) rather than a user-facing error, since `precision` is unreachable from public APIs: it always comes from a `TimestampNTZNanosType` / `TimestampLTZNanosType`, which can only be constructed with `p in [7, 9]`. Added a `DateTimeUtilsSuite` test covering `p in {6, 10}`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3351737703
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -158,6 +158,109 @@ sealed trait TimestampFormatter extends Serializable {
// did not fail if timestamp contained zone-id or zone-offset component and instead ignored it.
parseWithoutTimeZone(s, true)
+ /**
+ * Parses a timestamp in a string and converts it to a [[TimestampNanosVal]] (epoch microseconds
+ * plus a sub-microsecond remainder in `[0, 999]`) for `TIMESTAMP_LTZ(precision)`. Fractional
+ * digits beyond `precision` are truncated (floored), matching the cast/parse rule used by the
+ * microsecond path and `SparkDateTimeUtils`.
+ *
+ * @param s
+ * \- string with timestamp to parse
+ * @param precision
+ * \- the target fractional-second precision in `[7, 9]`
+ * @return
+ * the parsed value as a [[TimestampNanosVal]].
+ */
+ @throws(classOf[ParseException])
+ @throws(classOf[DateTimeParseException])
+ @throws(classOf[DateTimeException])
+ def parseNanos(s: String, precision: Int): TimestampNanosVal
+
+ /**
+ * Optional counterpart of [[parseNanos]]. The result is `None` on invalid input.
+ */
+ def parseNanosOptional(s: String, precision: Int): Option[TimestampNanosVal] =
+ try {
+ Some(parseNanos(s, precision))
+ } catch {
+ case _: Exception => None
+ }
Review Comment:
Good catch - this is a real inconsistency. The legacy formatters override the strict `parseNanos` / `parseWithoutTimeZoneNanos` to throw the user-facing `UNSUPPORTED_FEATURE.TIMESTAMP_NANOS_WITH_LEGACY_TIME_PARSER`, but the `*Optional` variants inherited the trait default, which catches `case _: Exception => None`. Since `legacyNanosUnsupported()` is a (non-fatal) `SparkUnsupportedOperationException`, the optional path swallowed it and returned `None` - so under the LEGACY policy the strict path errored while the optional path silently yielded null. That conflates genuinely invalid input (legitimately `None`) with an unsupported config combination (LEGACY + nanos), which should surface loudly so the user knows to switch to CORRECTED.
Rather than re-add a throwing override to every formatter and rely on remembering it for future ones, I removed the defaults entirely: `parseNanosOptional` and `parseWithoutTimeZoneNanosOptional` are now abstract in the (sealed) `TimestampFormatter` trait, so every implementer must consciously decide. The legacy formatters implement them by throwing `legacyNanosUnsupported()`; the `Default` / `Iso8601` formatters already implement them with the `None`-on-invalid-input behavior (their `parseNanos` doesn't throw the unsupported error, so that's correct there). Because the trait is sealed, the compiler guarantees completeness file-locally with no external burden. Extended the LEGACY-policy test to assert both optional variants raise the error instead of returning `None`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3349171227
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -280,6 +439,16 @@ class Iso8601TimestampFormatter(
localDateTime.format(formatter)
}
+ override def formatNanos(v: TimestampNanosVal, precision: Int): String = {
Review Comment:
Good catch. `formatNanos` was LTZ-only - it always routes through `format(Instant)` and applies `zoneId`, so an NTZ value rendered under a non-UTC formatter zone came out shifted.
I added an explicit NTZ counterpart `formatWithoutTimeZoneNanos(v, precision)` that mirrors the microsecond `format(LocalDateTime)` path: it rebuilds the zone-independent local date-time via `timestampNanosToLocalDateTime`, floors sub-precision digits, and renders with the pattern only (no `zoneId`). `formatNanos` stays the LTZ method and its scaladoc now says so explicitly; the legacy formatters override the new method to throw the same `UNSUPPORTED_FEATURE.TIMESTAMP_NANOS_WITH_LEGACY_TIME_PARSER` error. Added a regression test under a `+01:00` zone asserting `formatWithoutTimeZoneNanos` doesn't shift while `formatNanos` does - which the previous all-UTC NTZ cases couldn't catch.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56295:
URL: https://github.com/apache/spark/pull/56295#issuecomment-4611271486
@stevomitric Could you review this PR, please.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #56295:
URL: https://github.com/apache/spark/pull/56295#discussion_r3347952067
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -280,6 +439,16 @@ class Iso8601TimestampFormatter(
localDateTime.format(formatter)
}
+ override def formatNanos(v: TimestampNanosVal, precision: Int): String = {
Review Comment:
formatNanos always goes through format(Instant), so it's LTZ-only - applies zoneId. An NTZ value (UTC-grid wall clock) formatted with a non-UTC zone comes out shifted. Micro path has a separate format(LocalDateTime) for exactly this. Can we add an NTZ variant via timestampNanosToLocalDateTime (or doc it as LTZ-only + guard NTZ)? Tests don't catch it since NTZ cases all use UTC.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -227,6 +308,42 @@ class Iso8601TimestampFormatter(
} catch checkParsedDiff(s, legacyFormatter.parse)
}
+ // `checkParsedDiff` only uses the legacy parse to decide whether to raise an upgrade exception
+ // and never returns its result, so the legacy formatter (microsecond-only) is fine here even on
+ // the nanos path. The returned `TimestampNanosVal.ZERO` is discarded.
+ protected def legacyNanosParse(str: String): TimestampNanosVal = {
+ legacyFormatter.parse(str)
+ TimestampNanosVal.ZERO
+ }
+
+ override def parseNanosOptional(s: String, precision: Int): Option[TimestampNanosVal] = {
+ try {
+ val parsePosition = new ParsePosition(0)
+ val parsed = formatter.parseUnresolved(s, parsePosition)
+ if (parsed != null && s.length == parsePosition.getIndex) {
+ Some(extractNanos(parsed, precision))
+ } else {
+ None
+ }
+ } catch {
+ case NonFatal(_) => None
+ }
+ }
+
+ private def extractNanos(parsed: TemporalAccessor, precision: Int): TimestampNanosVal = {
Review Comment:
Default validates pā[7,9] (string helpers throw), but the Iso8601 path calls instantToTimestampNanos which doesn't - p=6/10 silently keep all 3 sub-micro digits. Worth a require so both subclasses enforce the contract the same way.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/TimestampFormatter.scala:
##########
@@ -38,15 +38,14 @@ import org.apache.spark.sql.errors.ExecutionErrors
import org.apache.spark.sql.internal.LegacyBehaviorPolicy._
import org.apache.spark.sql.internal.SqlApiConf
import org.apache.spark.sql.types.{Decimal, TimestampNTZType}
-import org.apache.spark.unsafe.types.UTF8String
+import org.apache.spark.unsafe.types.{TimestampNanosVal, UTF8String}
sealed trait TimestampFormatter extends Serializable {
/**
* Parses a timestamp in a string and converts it to microseconds.
*
- * @param s
- * \- string with timestamp to parse
+ * @param s String with timestamp to parse
Review Comment:
The @param reflow touches a bunch of pre-existing methods ā adds diff noise. Fine to keep for consistency, but could split if you want a cleaner diff.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57162][SQL] Add nanosecond-aware TimestampFormatter for parsing and formatting TimestampNanosVal [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56295:
URL: https://github.com/apache/spark/pull/56295#issuecomment-4619799747
Merging to master/4.x. Thank you, @HyukjinKwon @uros-b @stevomitric for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org