sunchao commented on code in PR #5177:
URL: https://github.com/apache/datafusion-comet/pull/5177#discussion_r3882446899
##########
spark/src/test/scala/org/apache/comet/parquet/ParquetReadSuite.scala:
##########
@@ -261,6 +261,56 @@ abstract class ParquetReadSuite extends CometTestBase {
}
}
+ test("TIMESTAMP_MILLIS overflow fails in native scan") {
+ // Spark routes both TimestampType and TimestampNTZType through
LongAsMicrosUpdater:
+ //
https://github.com/apache/spark/blob/v4.2.0/sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java#L140-L164
+ // The updater calls checked millisToMicros for direct and dictionary
values:
+ //
https://github.com/apache/spark/blob/v4.2.0/sql/core/src/main/java/org/apache/spark/sql/execution/datasources/parquet/ParquetVectorUpdaterFactory.java#L800-L833
+ // Matches Spark's positive and negative overflow cases:
+ //
https://github.com/apache/spark/blob/v4.2.0/sql/core/src/test/resources/sql-tests/inputs/timestamp.sql#L74-L83
+ def isOverflow(error: Throwable): Boolean =
+ Iterator
+ .iterate(error)(_.getCause)
+ .takeWhile(_ != null)
+ .exists(cause =>
Option(cause.getMessage).exists(_.toLowerCase.contains("overflow")))
+
+ Seq(false, true).foreach { dictionaryEnabled =>
+ Seq(92233720368547758L, -92233720368547758L).foreach { millis =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "part-r-0.parquet")
+ val schema = MessageTypeParser.parseMessageType("""
+ |message root {
+ | optional int64 ts(TIMESTAMP_MILLIS);
+ | optional int64 ts_ntz(TIMESTAMP(MILLIS,false));
+ |}
+ |""".stripMargin)
+ val writer = createParquetWriter(schema, path, dictionaryEnabled)
+ val record = new SimpleGroup(schema)
+ record.add(0, millis)
+ record.add(1, millis)
+ writer.write(record)
Review Comment:
**[P3] Exercise dictionary encoding in the overflow regression**
With this single row, Parquet 1.16.0 writes PLAIN pages even when
`dictionaryEnabled=true`, so the dictionary-enabled iteration does not cover a
dictionary read. I reproduced this for both timestamp columns and both overflow
signs. Could we write repeated rows and assert
`getEncodingStats.hasDictionaryEncodedPages`? A temporary 16-row variant
produced dictionary pages and passed this regression on Spark 4.1.3 with ANSI
both on and off.
--
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]
