andygrove opened a new pull request, #5840: URL: https://github.com/apache/datafusion-comet/pull/5840
## Which issue does this PR close? Closes #5836. ## Rationale for this change iceberg-java renders a `float` or `double` partition value with `Float.toString` / `Double.toString`: a fractional digit is always present, and the value switches to scientific notation outside `[1e-3, 1e7)`. Comet's partition-path renderer overrode the arms where iceberg-rust disagrees with iceberg-java but deliberately left float and double delegating, on the grounds that the divergence was cosmetic. It is not. Rust's `Display` never uses an exponent, so `Double.MAX_VALUE` renders as 309 digits and `Double.MIN_VALUE` as 324. A directory name that long exceeds the 255-byte limit on a single path component, and the write fails: ``` CometNativeException: Unexpected => Failed to finish parquet writer., source: ... path: .../d=1797693134862315700000000...0000/00000-00007-....parquet => invalid filename, source: File name too long (os error 36) ``` That is `TestSparkDataFile.testValueConversionWithEmptyStats` and `.testValueConversionPartitionedTable` failing on all four Iceberg versions in the #5677 run. Below the length limit the directory name still diverges from iceberg-java's, so a table written through both writers has two spellings of the same partition. Comet already implements Java's rules: `cast(float as string)` needs exactly the same rendering, and the `cast_float_to_string!` macro has spelled it since it was written. ## What changes are included in this PR? - `native/spark-expr/src/conversion_funcs/numeric.rs`: extract the formatting out of the `cast_float_to_string!` macro into `write_java_float_string`, generic over a `JavaFloatString` trait implemented for `f32` and `f64`, and export both from the crate. The macro becomes `spark_cast_float_to_utf8`, a generic function over the arrow float types. The cast still formats straight into the string builder with no per-row allocation; the coefficient inspection the scientific-notation branch needs now uses a stack buffer rather than a reused `String`, so the shared function does not have to take scratch space as a parameter. - `native/core/src/execution/operators/iceberg_partition_path.rs`: render `float` and `double` partition values through that function instead of delegating to iceberg-rust, and update the module documentation, which previously recorded the divergence as accepted. Two behaviours are unchanged and worth naming: this is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value, and only the smallest subnormal is corrected for. Iceberg deprecated float and double partitioning in 1.3, so this matters for tables that already carry such a field. ## How are these changes tested? - New Rust unit tests in `iceberg_partition_path.rs` pin the rendering of both widths against `Double.toString` / `Float.toString` output taken from a JDK: whole values, the boundaries of the plain-notation window, both extremes, NaN and the infinities. - A new case in `CometIcebergWriteActionSuite` writes a table partitioned by a `FLOAT` and a `DOUBLE` through the native writer and through the JVM writer and asserts the two produce the same partition directories, then pins the spelling of each. Without the fix it reproduces #5836 exactly, failing with `File name too long`. - `CometIcebergWriteActionSuite` (63 tests), `CometNativeCastSuite` (185 tests), the `expressions/cast` SQL file tests (21 tests) and the `datafusion-comet-spark-expr` unit tests (726 tests) all pass, covering the refactored cast path. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B -- 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]
