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/15 09:29:21 UTC
[PR] [SPARK-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
MaxGekk opened a new pull request, #56513:
URL: https://github.com/apache/spark/pull/56513
### What changes were proposed in this pull request?
Unify nanosecond timestamp (`TIMESTAMP_NTZ(p)` / `TIMESTAMP_LTZ(p)`) rendering in `HiveResult` onto the Types Framework, removing the inline duplicate renderer.
- `TimestampNanosTypeApiOps`: remove the `formatExternal(value, nested) = None` override so the Hive path shares each subclass's single-arg `formatExternal` renderer (the same one Row JSON uses). `nested` does not affect timestamp formatting.
- `HiveResult.toHiveStringDefault`: remove the inline `TimestampLTZNanosType` / `TimestampNTZNanosType` cases. The legacy path keeps no nanos handling, so a nanos value that somehow reaches it (only possible with the framework off, which the gating forbids) is unsupported rather than silently rendered.
- `TypeApiOps`: update the two-arg `formatExternal` scaladoc to reflect that Hive now shares the single-arg renderer.
### Why are the changes needed?
The nanosecond timestamp types are a Types Framework feature, implemented solely through it. External-value rendering for the framework is centralized in `TypeApiOps.formatExternal`, which already backs Row JSON (`Row.json` / `Row.prettyJson`).
The nanos ops previously overrode the two-arg `formatExternal(value, nested)` to return `None`, so `HiveResult` rendered nanos through inline pattern-matching in `toHiveStringDefault`. That duplicated the formatter logic and was documented in code as a temporary split "until nanos external rendering is unified across the zone-less (Row JSON) and zone-aware (Hive) paths".
The types are gated by `timestampNanosTypesEnabled = timestampNanosTypes.enabled && types.framework.enabled`, so a nanos column cannot exist while the framework is off; the inline cases are therefore dead code in that mode and redundant when the framework is on.
### Does this PR introduce _any_ user-facing change?
No. Nanos Hive output is identical: zone-aware LTZ, zone-independent NTZ, precision flooring, and trailing-zero trimming are all unchanged. Both the old inline path and the framework path ultimately call the same `TimestampFormatter` methods; the pre-flooring in the inline path is a no-op because values reaching Hive come from `executeCollectPublic()`, whose internal value is already stored floored to the column precision.
### How was this patch tested?
Existing `HiveResultSuite` SPARK-57257 tests cover precision 7/8/9, pre-1970 epochs, nested arrays/maps/structs, NULLs, and session-zone vs zone-independent rendering, and now exercise the framework path. Also ran `TimestampNanosTypeOpsSuite` and `RowJsonSuite`. All pass.
### 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-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #56513: [SPARK-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework
URL: https://github.com/apache/spark/pull/56513
--
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-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
Posted by "davidm-db (via GitHub)" <gi...@apache.org>.
davidm-db commented on code in PR #56513:
URL: https://github.com/apache/spark/pull/56513#discussion_r3413932548
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala:
##########
@@ -58,16 +58,14 @@ abstract class TimestampNanosTypeApiOps extends TypeApiOps with DataTypeErrorsBa
// ==================== String Formatting ====================
- // Row JSON (Row.json / Row.prettyJson) holds the external Row value (java.time.Instant for LTZ,
- // java.time.LocalDateTime for NTZ); each subclass overrides the single-arg formatExternal to
- // render it through the same formatter as its zone-aware cast-to-string, so Row JSON shows the
- // nanosecond value rather than silently truncating to microseconds via the legacy path.
-
- // The Hive result path (HiveResult.toHiveString) renders nanosecond timestamps through its own
- // zone-aware default formatter, so return None here to fall through to it rather than to the
- // subclass single-arg rendering. This is a temporary split until nanos external rendering is
- // unified across the zone-less (Row JSON) and zone-aware (Hive) paths.
- override def formatExternal(value: Any, nested: Boolean): Option[String] = None
+ // Both external-value consumers share the single-arg formatExternal each subclass overrides:
+ // - Row JSON (Row.json / Row.prettyJson) holds the external Row value (java.time.Instant for
+ // LTZ, java.time.LocalDateTime for NTZ).
+ // - The Hive result path (HiveResult.toHiveString) calls the two-arg overload, whose default
+ // delegates to the single-arg renderer; `nested` does not affect timestamp formatting.
+ // Each subclass renders the external value through the same formatter as its zone-aware
+ // cast-to-string, so both paths show the nanosecond value rather than silently truncating to
+ // microseconds via the legacy path.
Review Comment:
do we even need this comment now - it's free floating and to a random person reading the code it wouldn't refer to anything concrete, especially considering that we removed the actual override?
--
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-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56513:
URL: https://github.com/apache/spark/pull/56513#issuecomment-4716466760
All tests passed: https://github.com/MaxGekk/spark/runs/81612414862
Merging to master/4.x. Thank you, @HyukjinKwon 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
Re: [PR] [SPARK-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56513:
URL: https://github.com/apache/spark/pull/56513#discussion_r3414071022
##########
sql/api/src/main/scala/org/apache/spark/sql/types/ops/TimestampNanosTypeApiOps.scala:
##########
@@ -58,16 +58,14 @@ abstract class TimestampNanosTypeApiOps extends TypeApiOps with DataTypeErrorsBa
// ==================== String Formatting ====================
- // Row JSON (Row.json / Row.prettyJson) holds the external Row value (java.time.Instant for LTZ,
- // java.time.LocalDateTime for NTZ); each subclass overrides the single-arg formatExternal to
- // render it through the same formatter as its zone-aware cast-to-string, so Row JSON shows the
- // nanosecond value rather than silently truncating to microseconds via the legacy path.
-
- // The Hive result path (HiveResult.toHiveString) renders nanosecond timestamps through its own
- // zone-aware default formatter, so return None here to fall through to it rather than to the
- // subclass single-arg rendering. This is a temporary split until nanos external rendering is
- // unified across the zone-less (Row JSON) and zone-aware (Hive) paths.
- override def formatExternal(value: Any, nested: Boolean): Option[String] = None
+ // Both external-value consumers share the single-arg formatExternal each subclass overrides:
+ // - Row JSON (Row.json / Row.prettyJson) holds the external Row value (java.time.Instant for
+ // LTZ, java.time.LocalDateTime for NTZ).
+ // - The Hive result path (HiveResult.toHiveString) calls the two-arg overload, whose default
+ // delegates to the single-arg renderer; `nested` does not affect timestamp formatting.
+ // Each subclass renders the external value through the same formatter as its zone-aware
+ // cast-to-string, so both paths show the nanosecond value rather than silently truncating to
+ // microseconds via the legacy path.
Review Comment:
Good point - it was dangling after the override was removed. I dropped the free-floating comment and folded the Row JSON + Hive shared-renderer explanation into the class scaladoc, where it's anchored to a declaration. Done in 564364b.
--
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-57386][SQL] Render nanosecond timestamp types in HiveResult through the Types Framework [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56513:
URL: https://github.com/apache/spark/pull/56513#issuecomment-4706492828
@davidm-db @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