parthchandra commented on PR #6238: URL: https://github.com/apache/datafusion-comet/pull/6238#issuecomment-5919314314
Notes (verified against the pinned iceberg-rust rev: the NaN visitor counts over the whole list/map child ignoring the parent offset window, so a sliced batch over-counts; structs are safe): - **[PR body "Not covered"]** — the disclosed gap, NaNs counted under a null list/map entry whose offset range is non-empty, is a real iceberg-rust vs iceberg-java divergence this fix doesn't close, independent of slicing. It currently lives only in the description; please file a tracking issue and link it, since prose caveats don't get followed up. - **`native/core/src/execution/operators/iceberg_write.rs:1201`** (`reaches_past`) — worth a one-line note that `offsets` always has length >= 1 for a valid list/map array, so indexing `[len - 1]` and `[0]` is safe even for a zero-row batch. Correct as written; a comment stops a future reader worrying about the empty case. - **`iceberg_write.rs:1726`** (the test region) — every test uses `double`; none uses `float` (Float32). Please add one `float` case (cheapest as an `array<float>` column). There's also no test with null list or map entries in a sliced batch (exactly the disclosed remaining divergence), and no `list<struct<double>>`. This bug only manifests on Iceberg 1.5.2/1.8.1, so the Rust test carries the load; the JVM regression needs the `run-iceberg-tests` label to actually prove it. -- 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]
