andygrove commented on code in PR #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729#discussion_r3944410995
##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1033,6 +1033,124 @@ class CometIcebergWriteActionSuite
}
}
+ // iceberg-java renders a `timestamptz` partition value with
+ // `DateTimeUtil.microsToIsoTimestamptz` and a `binary` one with base64;
iceberg-rust's
+ // `partition_to_path` renders the first as a `chrono` `DateTime<Utc>`
(space separator, " UTC"
+ // suffix) -- and outright panics on a pre-1970 value with a sub-second
part, since
+ // `microseconds_to_datetimetz` casts a negative remainder to `u32`
+ // (apache/datafusion-comet#5694) -- and the second as uppercase hex. Comet
renders the path
+ // itself; this pins the result against the layout iceberg-java's own writer
produces.
+ test("native acceleration: timestamptz and binary partition paths match
iceberg-java") {
+ assumeNativeAcceleration()
+ withIcebergCatalog { warehouseDir =>
+ // A UTC session zone makes the stored micros of each literal exact, so
the expected
+ // directory names below are not a function of the machine's zone.
+ withSQLConf("spark.sql.session.timeZone" -> "UTC") {
+ Seq("ts_path_native", "ts_path_jvm").foreach { table =>
+ spark.sql(s"""
+ CREATE TABLE $catalog.$ns.$table (id INT, ts TIMESTAMP, bin BINARY)
+ USING iceberg PARTITIONED BY (ts, bin)
+ """)
+ }
+ // Pre-epoch with a sub-second part (the panic case), pre-epoch on a
whole second, the
+ // epoch itself, and a post-epoch microsecond value.
+ val values =
+ "(1, TIMESTAMP '1969-12-31 23:59:58.5', X'0001FF'), " +
+ "(2, TIMESTAMP '1969-12-31 23:59:58', X'00'), " +
+ "(3, TIMESTAMP '1970-01-01 00:00:00', X''), " +
+ "(4, TIMESTAMP '2024-04-01 19:25:00.123456', X'FF')"
+
+ assertNativeWriteEngages("ts_path_native", Seq(1, 2, 3, 4)) {
+ spark.sql(s"INSERT INTO $catalog.$ns.ts_path_native VALUES $values")
+ }
+ spark.sql(s"INSERT INTO $catalog.$ns.ts_path_jvm VALUES $values")
+
+ val nativeDirs = partitionDirs(warehouseDir, "ts_path_native")
+ assert(nativeDirs == partitionDirs(warehouseDir, "ts_path_jvm"),
s"native: $nativeDirs")
Review Comment:
You're right, and I'd missed that iceberg-java's own rendering changed here
rather than just diverging from mine: 1.5.2's `humanTimestampWithZone` is
`ChronoUnit.MICROS.addTo(EPOCH, micros).toString()`, and 1.8 moved it to
`DateTimeUtil.microsToIsoTimestamptz`. While checking that I noticed 1.5.2
doesn't escape the partition field name either, which no Comet version has
reproduced, so it's the same version boundary.
I've gated just the JVM comparison on `icebergVersionAtLeast(1, 8)` and left
the pinned expectations and the readback unconditional, so 3.4 still covers the
pre-epoch write that used to panic. I chose to emit the 1.8+ spelling on every
profile rather than branch the renderer on the runtime version, since the
directory name is cosmetic and nothing parses it back, and said so in the
module docs and in the accepted-divergences list. Happy to make it follow the
runtime instead if you'd rather Comet and the JVM writer always agree on 3.4.
--
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]
