andygrove commented on PR #5779: URL: https://github.com/apache/datafusion-comet/pull/5779#issuecomment-5592115154
Thanks — both parts of the P2 reproduce, and the timestamp one is worse than the message mismatch. Pushed a fix (rebased onto main first, since `iceberg_partition_path.rs` landed after my branch point). `partition_to_path` moves out of `CometLocationGenerator` as a free function taking the resolved partition type, and the error now goes through it instead of `PartitionKey::to_path()`. Both callers already hold that partition type — the location generator resolves it at task start, `ClusteredBatchSplitter` needs it to read partition values — so nothing new gets resolved or plumbed. I confirmed your source analysis against the real pipeline rather than just taking it on inference. Reverting the renderer and re-running: - binary — `partition 'part=0001FF'` instead of `part=AAH%2F`, as you said. - negative fractional `timestamptz` — panics at `iceberg-rust/spec/values/temporal.rs:103` before `CometError::IllegalState` is constructed. That reproduces natively, not just on paper: the Scala test drives a real Spark write and the task dies with `Comet native panic` in `temporal.rs`. So this was a crash on that path, not a wrong string. Test coverage per your ask: - two Rust tests over the real iceberg-rust writer stack, one per case (`unclustered_binary_partition_renders_like_iceberg_java`, `unclustered_negative_timestamptz_partition_renders_like_iceberg_java`); - a Scala parity test on a `(id, ts TIMESTAMP, bin BINARY)` table with interleaved values, comparing the native writer's exception class, cause-chain depth and message against iceberg-java's own writer on the same runtime, plus an explicit pin on `ts=1969-12-31T23%3A59%3A58.5%2B00%3A00/bin=AAH%2F`. It asserts the aborted write really went through `CometIcebergWriteExec`, so it cannot pass by falling back. The Scala test is gated on `icebergVersionAtLeast(1, 8)` for the same reason the existing partition-path test gates its byte-for-byte comparison: 1.5.x on the Spark 3.4 profile spells a `timestamptz` partition value `1969-12-31T23:59:58.500Z`, so the two writers cannot agree there. On the retained `PartitionKey` clone you flagged under Performance — agreed it is a per-run cost. It sits on top of the spec clone `ClusteredBatchSplitter::partition_key` already pays each run (`PartitionKey::new` takes the spec by value), so it doubles an existing per-run allocation rather than adding a new class of cost, and the run boundary it sits on is one that closes a parquet file. I did not measure it and am not claiming otherwise. Local runs on this head: 147 tests across five Iceberg suites, `CometIcebergNativeSuite` 100 passed / 1 canceled, Rust core 330 and jni-bridge 28, clippy and rustfmt clean, `test-compile` on spark-3.4/scala-2.12, 3.5, 4.0 and the default 4.1. -- 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]
