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: [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]
