You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stevomitric (via GitHub)" <gi...@apache.org> on 2026/09/10 10:18:48 UTC
[PR] [WIP][SPARK-56822][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
stevomitric opened a new pull request, #58693:
URL: https://github.com/apache/spark/pull/58693
### What changes were proposed in this pull request?
Extend the nanosecond-timestamp Parquet read path (which already reads INT64 TIMESTAMP(MICROS)) to also read a legacy INT96 timestamp column as a nanosecond type - the read side of widening a TIMESTAMP(6) column stored as INT96 to nanosecond precision.
INT96 carries no logical unit and decodes to microseconds (ParquetRowConverter.binaryToSQLTimestamp), so each value is promoted to the internal (epochMicros, nanosWithinMicro = 0) representation - the same promotion the MICROS path uses. The requested type's family decides handling, mirroring the INT96 arms of ParquetRowConverter: the LTZ family applies the INT96 Julian rebase and any timezone conversion; the NTZ family applies neither. INT96 spans the full date range, so the promotion is range-complete.
### Why are the changes needed?
INT96 is the default on-disk encoding for LTZ timestamps, so without this an INT96-encoded TIMESTAMP(6) column could not be widened to nanoseconds and read back. This complements the INT64 TIMESTAMP(MICROS) read support.
### Does this PR introduce _any_ user-facing change?
Yes, behind the preview flag spark.sql.timestampNanosTypes.enabled, reading an INT96 timestamp column under a nanosecond timestamp type now succeeds (previously threw).
### How was this patch tested?
extended ParquetTypeWideningSuite.
### Was this patch authored or co-authored using generative AI tooling?
Co-authored-by: Claude Code 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] [WIP][SPARK-56822][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58693:
URL: https://github.com/apache/spark/pull/58693#discussion_r3980255434
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java:
##########
@@ -227,6 +227,17 @@ public ParquetVectorUpdater getUpdater(ColumnDescriptor descriptor, DataType spa
int96RebaseTz);
}
}
+ } else if (sparkType instanceof TimestampNTZNanosType) {
Review Comment:
this is reachable only through `.schema(...)`. default is int64 for timestamp ntz. I've added a test.
--
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] [WIP][SPARK-56822][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58693:
URL: https://github.com/apache/spark/pull/58693#discussion_r3978223650
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetTypeWideningSuite.scala:
##########
@@ -225,9 +226,30 @@ class ParquetTypeWideningSuite
}
}
+ // The same widening from a legacy INT96 timestamp column. INT96 has no logical unit; it decodes
+ // to microseconds and is promoted to (epochMicros, 0). INT96 is only produced for the LTZ family;
+ // the write/read INT96 rebase are pinned to CORRECTED so proleptic-Gregorian values round-trip.
for {
- outputTimestampType <-
- Seq(ParquetOutputTimestampType.INT96, ParquetOutputTimestampType.TIMESTAMP_MILLIS)
+ toType: DataType <- Seq(
+ TimestampLTZNanosType(TimestampLTZNanosType.NANOS_PRECISION),
+ TimestampLTZNanosType(7))
+ }
+ test(s"parquet widening conversion TimestampType (int96) -> $toType") {
+ withSQLConf(
+ SQLConf.TIMESTAMP_NANOS_TYPES_ENABLED.key -> "true",
+ SQLConf.PARQUET_OUTPUT_TIMESTAMP_TYPE.key -> ParquetOutputTimestampType.INT96.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.key -> LegacyBehaviorPolicy.CORRECTED.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.key -> LegacyBehaviorPolicy.CORRECTED.toString) {
Review Comment:
Tests pin INT96 rebase to CORRECTED and use 2020 / 5138. The MICROS widening loop documents pre-1582 LEGACY rebase; INT96 needs the same, plus EXCEPTION failIfRebase. Dictionary decode is implemented, keep checkAllParquetReaders dict on/off.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java:
##########
@@ -227,6 +227,17 @@ public ParquetVectorUpdater getUpdater(ColumnDescriptor descriptor, DataType spa
int96RebaseTz);
}
}
+ } else if (sparkType instanceof TimestampNTZNanosType) {
Review Comment:
Spark writes NTZ as INT64 MICROS, but the read path still accepts INT96 as TimestampNTZNanosType. If that widening is intentional, please add a read test; otherwise drop the arm.
--
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-59401][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58693:
URL: https://github.com/apache/spark/pull/58693#issuecomment-5698867396
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/51517b99d9e64599c310d83cea155e5af0602f6a
- merged into branch-4.x https://github.com/apache/spark/commit/1f519ff129e1da6ad3e6cbf0d43f8991917119be
*Posted by `merge_spark_pr.py`*
--
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-59401][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #58693:
URL: https://github.com/apache/spark/pull/58693#discussion_r3983032232
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetTypeWideningSuite.scala:
##########
@@ -225,9 +226,36 @@ class ParquetTypeWideningSuite
}
}
+ // The same widening from a legacy INT96 timestamp column. INT96 has no logical unit; each value
+ // decodes to micros (binaryToSQLTimestamp) and is promoted to (epochMicros, nanosWithinMicro).
+ // Spark writes micro-aligned INT96, so a round-trip carries no sub-microsecond digits; foreign
+ // nanosecond INT96 (whose sub-micro remainder is preserved) is covered by
+ // TimestampNanosParquetOpsSuite. INT96 is only produced for the LTZ family. Both a CORRECTED and
+ // a LEGACY (Julian) INT96 rebase are exercised: LEGACY over the pre-1582 value actually runs the
+ // INT96 read rebase, which the earlier CORRECTED-only pinning skipped.
for {
- outputTimestampType <-
- Seq(ParquetOutputTimestampType.INT96, ParquetOutputTimestampType.TIMESTAMP_MILLIS)
+ toType: DataType <- Seq(
+ TimestampLTZNanosType(TimestampLTZNanosType.NANOS_PRECISION),
+ TimestampLTZNanosType(7))
+ int96RebaseMode <- Seq(LegacyBehaviorPolicy.CORRECTED, LegacyBehaviorPolicy.LEGACY)
+ }
+ test(s"parquet widening conversion TimestampType (int96, $int96RebaseMode) -> $toType") {
+ withSQLConf(
+ SQLConf.TIMESTAMP_NANOS_TYPES_ENABLED.key -> "true",
+ SQLConf.PARQUET_OUTPUT_TIMESTAMP_TYPE.key -> ParquetOutputTimestampType.INT96.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.key -> int96RebaseMode.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.key -> int96RebaseMode.toString) {
+ checkAllParquetReaders(
+ values = Seq(
+ "2020-01-01 12:34:56.123456", "1312-02-27 01:02:03.654321", "5138-11-16 09:46:40"),
+ fromType = TimestampType,
+ toType = toType,
+ expectError = false)
+ }
+ }
+
Review Comment:
`ParquetVectorUpdaterFactory.java` (`Int96AsTimestampNanosUpdater`) / `ParquetTypeWideningSuite.scala` -- The new vectorized updater is exercised end-to-end only for the LTZ family with micro-aligned, Spark-written values (CORRECTED + LEGACY rebase, dictionary on/off, vectorized on/off, via `checkAllParquetReaders`). Its sub-microsecond recovery -- the PR's headline behavior ("a foreign nanosecond INT96 is not silently floored to micros") -- plus its NTZ arm and its EXCEPTION (`failIfRebase`) arm have no *vectorized* coverage: Spark only writes micro-aligned INT96, so the widening loop always yields `nanosWithinMicro == 0`, and those corners are pinned only on the row-based converter (`TimestampNanosParquetOpsSuite.decodeInt96`), while the default vectorized reader runs the parallel `putInt96AsNanos` implementation with a non-zero remainder untested. This maps onto uros-b's inline ask to test the vectorized NTZ arm (`ParquetVectorUpdaterFactory.java:232`), which was answered with
a row-based unit test rather than one that drives the vectorized updater. A vectorized-reader test carrying a hand-crafted sub-microsecond INT96 (and an NTZ variant) would pin the default path's headline behavior. The code itself is verified correct against the row path and the existing INT96->Timestamp updaters, so this is a coverage-rigor gap on a new code path, not a suspected defect.
--
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] [WIP][SPARK-56822][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on code in PR #58693:
URL: https://github.com/apache/spark/pull/58693#discussion_r3980072050
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetTypeWideningSuite.scala:
##########
@@ -225,9 +226,30 @@ class ParquetTypeWideningSuite
}
}
+ // The same widening from a legacy INT96 timestamp column. INT96 has no logical unit; it decodes
+ // to microseconds and is promoted to (epochMicros, 0). INT96 is only produced for the LTZ family;
+ // the write/read INT96 rebase are pinned to CORRECTED so proleptic-Gregorian values round-trip.
for {
- outputTimestampType <-
- Seq(ParquetOutputTimestampType.INT96, ParquetOutputTimestampType.TIMESTAMP_MILLIS)
+ toType: DataType <- Seq(
+ TimestampLTZNanosType(TimestampLTZNanosType.NANOS_PRECISION),
+ TimestampLTZNanosType(7))
+ }
+ test(s"parquet widening conversion TimestampType (int96) -> $toType") {
+ withSQLConf(
+ SQLConf.TIMESTAMP_NANOS_TYPES_ENABLED.key -> "true",
+ SQLConf.PARQUET_OUTPUT_TIMESTAMP_TYPE.key -> ParquetOutputTimestampType.INT96.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_WRITE.key -> LegacyBehaviorPolicy.CORRECTED.toString,
+ SQLConf.PARQUET_INT96_REBASE_MODE_IN_READ.key -> LegacyBehaviorPolicy.CORRECTED.toString) {
Review Comment:
done, the INT96 read now runs under both CORRECTED and LEGACY rebase over a pre-1582 value.
--
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-59401][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #58693: [SPARK-59401][SQL] Read INT96 timestamp columns as nanosecond timestamps
URL: https://github.com/apache/spark/pull/58693
--
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-59401][SQL] Read INT96 timestamp columns as nanosecond timestamps [spark]
Posted by "stevomitric (via GitHub)" <gi...@apache.org>.
stevomitric commented on PR #58693:
URL: https://github.com/apache/spark/pull/58693#issuecomment-5634646752
Fix now, added a vectorized-reader test that reads a foreign INT96 carrying sub-microsecond digits. It covers both NTZ and LTZ, dictionary on/off, plus the EXCEPTION-rebase case. `putInt96AsNanos` is now tested end-to-end through the default reader, not just the row-based unit tests.
--
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