You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/05 23:31:05 UTC
[PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
andygrove opened a new pull request, #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729
## Which issue does this PR close?
Closes #5691.
Closes #5694.
Closes #5693 (same root cause as #5691, one step further along).
## Rationale for this change
Two native Iceberg write panics, both crossing the JNI boundary as a `CometNativeException`
rather than surfacing as an error Comet could fall back on or report cleanly.
**#5691 / #5693 — evolved partition spec.** iceberg-java's `UpdatePartitionSpec` keeps a dropped
partition field in a format-version-1 spec as a `void` transform so its field id survives, and
`PartitionSpec#isUnpartitioned` means "every field is `void`", not "no fields" — on the Rust side
too. The next write therefore routes through `UnpartitionedWriter`, which stamps every data file
with an empty partition struct, while `ManifestWriter` derives one partition summary per spec
field and `zip_eq`s the two:
```
itertools: .zip_eq() reached end of one iterator before the other
at <iceberg::spec::manifest::writer::ManifestWriter>::construct_partition_summaries
```
Dropping the `void` field's source column afterwards then broke the manifest's `partition_type`
resolution as well (`No column with source column id 2 in schema`), which is #5693.
**#5694 — `timestamptz` partition path.** iceberg-rust's `microseconds_to_datetimetz` takes
`micros % 1_000_000` — negative for a pre-1970 value — casts it to `u32` and multiplies by 1000,
then unwraps the `None` that `DateTime::from_timestamp` returns for the resulting out-of-range
nanosecond count. `RandomData` in Iceberg's own tests generates timestamps as
`random.nextLong() % FIFTY_YEARS_IN_MICROS`, so the value reaching the conversion is a legitimate
pre-epoch timestamp; the conversion is simply wrong for negative inputs. Still present on
iceberg-rust `main`, so there is nothing to bump the pin to.
## What changes are included in this PR?
**#5691 / #5693.** `encode_data_files_as_manifest` now encodes the per-task transport manifest
against a field-less spec of the same spec id whenever `is_unpartitioned()` holds (a no-op in the
common already-field-less case), so the manifest's partition arity matches the empty partition
struct the writer produced. Nothing downstream loses information: the JVM re-reads this manifest
with the spec embedded in its own Avro metadata, then rebuilds each `DataFile` against the real
output spec, whose `DataFiles.Builder` drops partition data outright for an unpartitioned spec —
so the manifest that reaches storage carries exactly what iceberg-java's own writer would have
committed. Dropping the fields also skips the `partition_type` resolution that #5693 tripped over.
**#5694.** New `CometLocationGenerator` (`native/core/src/execution/operators/iceberg_partition_path.rs`)
replaces iceberg-rust's `DefaultLocationGenerator`. It lays files out identically but renders the
partition path itself, mirroring iceberg-java's `PartitionSpec#partitionToPath`. Per-field it
delegates to `Transform::to_human_string` and overrides only the arms where iceberg-rust disagrees
with iceberg-java:
| Iceberg type | iceberg-java | iceberg-rust |
| --- | --- | --- |
| `timestamp` | `1969-12-31T23:59:58.5` | `1969-12-31 23:59:58.500` |
| `timestamptz` | `1969-12-31T23:59:58.5+00:00` | panics on a negative value with a sub-second part; otherwise `1969-12-31 23:59:58.500 UTC` |
| `binary` / `fixed` | base64 | uppercase hex |
I widened it past `timestamptz` because a function whose stated job is iceberg-java parity should
not knowingly leave two adjacent arms wrong, and Iceberg's own `TestSparkDataFile` SPEC (the
#5694 reproducer) partitions on `binary` and `timestamp` as well as `timestamptz`. The name/value
escaping is unchanged: `form_urlencoded` leaves exactly the byte set `URLEncoder.encode(s, UTF_8)`
leaves.
The generator resolves the partition type once per task instead of per file, which turns a spec it
could not render into an error at task start rather than a panic inside the infallible
`LocationGenerator::generate_location`.
One deliberate gap, documented in the code and in `iceberg-writes.md`: `float` and `double` still
delegate, so a float partition directory reads `f=1` where iceberg-java writes `f=1.0`. Matching
Java means porting `Float.toString`/`Double.toString` — the same port Comet's
`cast(float as string)` needs — which is much more than a partition directory name warrants, and
unlike `timestamptz` it does not panic. Distinct partition values still get distinct directories,
and no reader parses these names.
## How are these changes tested?
New tests, all of which I confirmed **fail against the pre-fix native library with the exact
panics from the issues** (`zip_eq() reached end of one iterator before the other` for #5691;
`attempt to multiply with overflow` for #5694 — the debug-build face of the release-build
`Option::unwrap()` in the issue) and pass with it:
- `CometIcebergWriteActionSuite`, two end-to-end parity tests that write the same data twice, once
natively and once through iceberg-java's writer, and compare:
- `timestamptz and binary partition paths match iceberg-java` — the full set of committed
partition directories must be equal between the two tables, plus pinned expectations for the
pre-epoch sub-second value, the epoch, and a microsecond-precision value.
- `writes after a V1 partition field is dropped match iceberg-java` — walks the exact sequence
from #5691 (`ADD PARTITION FIELD` → write → `DROP PARTITION FIELD` → write → `DROP COLUMN` →
write) and compares rows and the committed manifests' `partition_spec_id` /
`partition_summaries`.
- `iceberg_write.rs`: `void_only_spec_write_round_trips_through_the_manifest`,
`void_field_with_a_dropped_source_column_still_writes`, and
`pre_epoch_timestamptz_partition_gets_a_java_shaped_directory` drive the real iceberg-rust
writer stack.
- `iceberg_partition_path.rs`: unit tests pinning each rendering against the strings the Java
source produces, including `ISO_LOCAL_DATE_TIME`'s always-print-seconds and
strip-trailing-zeros-fraction behaviour (`.5`, not `.500000`), `EXCEEDS_PAD` year formatting,
and timestamps past chrono's year-262143 calendar ceiling (which `i64` micros can reach and
iceberg-java has no ceiling for, hence the hand-rolled `civil_from_days`).
Full runs: the whole native workspace (`cargo test --workspace`) and all seven Comet Iceberg
suites (233 tests) pass.
Not verified here: Iceberg's own gradle suites (`TestAlterTablePartitionFields`,
`TestSparkDataFile`). Gradle cannot start in my environment — it fails binding its
`FileLockContentionHandler` socket. Two notes for reviewers on that:
1. The `iceberg_spark_test_1.11` workflow will not catch these either, because
`dev/diffs/iceberg/1.11.0.diff` does not set `spark.comet.iceberg.write.enabled` — the native
write path is only reachable in those suites once #5677 turns it on. That is why I put the
equivalent iceberg-java-parity coverage in Comet's own suite instead.
2. The panic in #5694 is upstream in iceberg-rust, unfixed on `main`; this PR routes around it
rather than fixing it there. Happy to file the upstream issue and add the reference here.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729#discussion_r3942777915
##########
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:
### Correctness
[P2] Make timestamp-path parity conditional on the Iceberg formatter version
This equality fails in the supported Spark 3.4 profile, which pins Iceberg 1.5.2. Its `TransformUtil.humanTimestampWithZone` uses `OffsetDateTime.toString()`, so the JVM writes `1969-12-31T23:59:58.500Z` and `1970-01-01T00:00Z`, while this patch writes `1969-12-31T23:59:58.5+00:00` and `1970-01-01T00:00:00+00:00`. The [Spark 3.4 scan job](https://github.com/apache/datafusion-comet/actions/runs/33998881999/job/101397610298) fails at this assertion with exactly those sets. Make the byte-for-byte JVM comparison version-aware, while retaining the pre-epoch write and readback checks on 3.4, and qualify the corresponding documentation claim. The older layout difference does not itself indicate incorrect stored values.
##########
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")
+ assert(
+ nativeDirs.contains("ts=1969-12-31T23%3A59%3A58.5%2B00%3A00/bin=AAH%2F"),
+ s"native: $nativeDirs")
+ assert(
+ nativeDirs.contains("ts=1970-01-01T00%3A00%3A00%2B00%3A00/bin="),
+ s"native: $nativeDirs")
+ assert(
+ nativeDirs.contains("ts=2024-04-01T19%3A25%3A00.123456%2B00%3A00/bin=%2Fw%3D%3D"),
+ s"native: $nativeDirs")
+
+ // Values round-trip through both readers regardless of how the path was spelled.
+ Seq("true", "false").foreach { cometEnabled =>
+ withSQLConf(CometConf.COMET_ENABLED.key -> cometEnabled) {
+ val rows =
+ spark.sql(s"SELECT id, ts FROM $catalog.$ns.ts_path_native ORDER BY id").collect()
+ assert(
+ rows.toSeq ==
+ spark
+ .sql(s"SELECT id, ts FROM $catalog.$ns.ts_path_jvm ORDER BY id")
+ .collect()
+ .toSeq,
+ s"comet=$cometEnabled: $rows")
+ }
+ }
+ }
+ }
+ }
+
+ // iceberg-java's `UpdatePartitionSpec` keeps a dropped partition field in a format-version-1
+ // spec as a `void` transform so its field id survives, and `PartitionSpec#isUnpartitioned` is
+ // "every field is void", not "no fields". The next write therefore runs through the
+ // unpartitioned writer -- which stamps an empty partition struct -- against a spec whose fields
+ // are non-empty, and iceberg-rust's `ManifestWriter` used to `zip_eq` the two and panic across
+ // the JNI boundary (apache/datafusion-comet#5691). Dropping the source column afterwards then
+ // broke the manifest's `partition_type` resolution as well (apache/datafusion-comet#5693).
+ test("native acceleration: writes after a V1 partition field is dropped match iceberg-java") {
+ assumeNativeAcceleration()
+ withIcebergCatalog { _ =>
+ Seq("evolved_native", "evolved_jvm").foreach { table =>
+ spark.sql(s"""
+ CREATE TABLE $catalog.$ns.$table (id INT, region STRING)
+ USING iceberg TBLPROPERTIES ('format-version'='1')
+ """)
+ }
+
+ def evolve(table: String, write: (String, Seq[Int]) => Unit): Unit = {
+ write("(1, 'us')", Seq(1))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table ADD PARTITION FIELD region AS region_part")
+ write("(2, 'eu')", Seq(1, 2))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table DROP PARTITION FIELD region_part")
+ // The spec is now void-only: the #5691 shape.
+ write("(3, 'ap')", Seq(1, 2, 3))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table DROP COLUMN region")
+ // The void field's source column is gone: the #5693 shape.
+ write("(4)", Seq(1, 2, 3, 4))
Review Comment:
### Correctness
[P2] Separate the dropped-source regression from runtimes that cannot commit it
The unconditional final stage fails in every Spark 3.4, 3.5 and 4.0 scan job. Their pinned Iceberg 1.5.2/1.8.1 runtimes fail resolving the dropped source in `PartitionSpec.partitionType`; 1.10.0 reaches commit but fails in `PartitionSpec.javaClasses` because the transform result type is null. The [Spark 4.0 failure](https://github.com/apache/datafusion-comet/actions/runs/33998881999/job/101397610203) traces through `SnapshotSummary` and `IcebergCommitExec`, so normalizing the native transport manifest does not avoid this JVM failure. Iceberg 1.11 handles the missing source, and the new test passes in the Spark 4.1 job. Split the source-column-drop scenario behind the supported Iceberg-version condition while keeping the preceding void-only writes covered on older profiles; otherwise this regression test leaves four scan jobs failing.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729#discussion_r3944411574
##########
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")
+ assert(
+ nativeDirs.contains("ts=1969-12-31T23%3A59%3A58.5%2B00%3A00/bin=AAH%2F"),
+ s"native: $nativeDirs")
+ assert(
+ nativeDirs.contains("ts=1970-01-01T00%3A00%3A00%2B00%3A00/bin="),
+ s"native: $nativeDirs")
+ assert(
+ nativeDirs.contains("ts=2024-04-01T19%3A25%3A00.123456%2B00%3A00/bin=%2Fw%3D%3D"),
+ s"native: $nativeDirs")
+
+ // Values round-trip through both readers regardless of how the path was spelled.
+ Seq("true", "false").foreach { cometEnabled =>
+ withSQLConf(CometConf.COMET_ENABLED.key -> cometEnabled) {
+ val rows =
+ spark.sql(s"SELECT id, ts FROM $catalog.$ns.ts_path_native ORDER BY id").collect()
+ assert(
+ rows.toSeq ==
+ spark
+ .sql(s"SELECT id, ts FROM $catalog.$ns.ts_path_jvm ORDER BY id")
+ .collect()
+ .toSeq,
+ s"comet=$cometEnabled: $rows")
+ }
+ }
+ }
+ }
+ }
+
+ // iceberg-java's `UpdatePartitionSpec` keeps a dropped partition field in a format-version-1
+ // spec as a `void` transform so its field id survives, and `PartitionSpec#isUnpartitioned` is
+ // "every field is void", not "no fields". The next write therefore runs through the
+ // unpartitioned writer -- which stamps an empty partition struct -- against a spec whose fields
+ // are non-empty, and iceberg-rust's `ManifestWriter` used to `zip_eq` the two and panic across
+ // the JNI boundary (apache/datafusion-comet#5691). Dropping the source column afterwards then
+ // broke the manifest's `partition_type` resolution as well (apache/datafusion-comet#5693).
+ test("native acceleration: writes after a V1 partition field is dropped match iceberg-java") {
+ assumeNativeAcceleration()
+ withIcebergCatalog { _ =>
+ Seq("evolved_native", "evolved_jvm").foreach { table =>
+ spark.sql(s"""
+ CREATE TABLE $catalog.$ns.$table (id INT, region STRING)
+ USING iceberg TBLPROPERTIES ('format-version'='1')
+ """)
+ }
+
+ def evolve(table: String, write: (String, Seq[Int]) => Unit): Unit = {
+ write("(1, 'us')", Seq(1))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table ADD PARTITION FIELD region AS region_part")
+ write("(2, 'eu')", Seq(1, 2))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table DROP PARTITION FIELD region_part")
+ // The spec is now void-only: the #5691 shape.
+ write("(3, 'ap')", Seq(1, 2, 3))
+ spark.sql(s"ALTER TABLE $catalog.$ns.$table DROP COLUMN region")
+ // The void field's source column is gone: the #5693 shape.
+ write("(4)", Seq(1, 2, 3, 4))
Review Comment:
Agreed, and thanks for separating 1.10 out. I'd assumed the `UnknownType` fallback added to `partitionType` covered it, but `javaClasses` only got the same fallback in 1.11, so 1.10 gets further and still fails. Since that failure is in the driver-side commit the native path shares with the stock one, gating is the only honest option: normalising the transport manifest can't rescue it.
Split the source-column drop into its own test behind `assume(icebergVersionAtLeast(1, 11))` and left the void-only stages unconditional, so #5691 itself stays covered on 3.4/3.5/4.0. Verified `CometIcebergWriteActionSuite` locally on spark-3.4 (Iceberg 1.5.2), spark-3.5 (1.8.1), spark-4.0 (1.10.0) and spark-4.1 (1.11.0), plus the other Iceberg suites in the `scans` group on 3.4 and 4.0.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
Posted by "flyovers-bot (via GitHub)" <gi...@apache.org>.
flyovers-bot commented on PR #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729#issuecomment-5583877689
<!-- flyovers:flyover -->
[](https://flyovers.dev/flyover/974106db-aba9-432b-9cac-977a5582b3ab)
**[Watch the flyover →](https://flyovers.dev/flyover/974106db-aba9-432b-9cac-977a5582b3ab)** · 11:41 · 6 chapters · 7 files
An animated walkthrough of this diff: context, the problem, the change, implementation care, code walkthrough, and impact.
<sub>ce3379a · updated automatically by flyovers</sub>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: native Iceberg write panics on an evolved partition spec and on a timestamptz partition path [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org