fix: render float and double Iceberg partition values like iceberg-java

[andygrove]

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 Native Iceberg write renders float/double partition values differently from iceberg-java, and fails with "File name too long" for large values #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.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B

[rich7420]

@andygrove thanks for the patch

[comphead]

Reviewed the fix and the extraction. The rendering logic is unchanged from the old cast_float_to_string! macro (I checked the -0.0, NaN/infinity, subnormal and plain-window branches line by line), the two new human_string arms match on both the partition field type and the literal variant so no other transform can reach them, and the String allocation in java_float_string really is off the hot path (partition_to_path is called per data file and from not_clustered_error). No blockers.

The notes below are reuse/scope/test-placement suggestions, not correctness objections.

[comphead]

Both impls spell this identically (self.abs().to_bits() == 1), so the method only exists because to_bits is not on num::Float. An associated value const removes the method and both bodies:

/// `Float.MIN_VALUE` / `Double.MIN_VALUE`.
const MIN_SUBNORMAL: Self;          // f32::from_bits(1) / f64::from_bits(1)
/// ...as Java spells it.
const MIN_SUBNORMAL_TEXT: &'static str;

Call site becomes abs == T::MIN_SUBNORMAL. from_bits is const well below the 1.94 MSRV. Renaming the string const also stops MIN_SUBNORMAL reading like the value rather than its spelling.

[andygrove]

Done. MIN_SUBNORMAL: Self via from_bits, spelling moved to MIN_SUBNORMAL_TEXT, method removed.

[comphead]

lib.rs re-exports with pub use conversion_funcs::*, so this makes JavaFloatString part of the published datafusion-comet-spark-expr API and lets a downstream crate implement it for its own type. The only external consumer needs f32/f64 -> String.

Either seal the trait (private supertrait) or export just the owned helper, e.g. pub fn java_float_string<T: JavaFloatString>(v: T) -> String. The second also lets iceberg_partition_path.rs drop its local wrapper and the JavaFloatString import.

[andygrove]

Sealed with a private supertrait and added the owned java_float_string next to the writer; core now imports only that.

[comphead]

The guarantee that holds here is stronger than the one stated, and does not depend on what bucket accepts: partition_to_path passes partition_type.fields()[index].field_type, i.e. the transform's result type, so bucket presents int and can never reach a Float/Double arm even for a float source column (iceberg-java did allow bucketing float/double before deprecating it in 1.3). truncate keeps the source type but has no float/double arm, so identity is indeed the only way in.

Suggest resting the comment on the result-type argument instead.

[andygrove]

Rewritten on the result-type argument. Checked Identity/Bucket/Truncate.canTransform at 1.8.1 and 1.11.0: bucket never accepted float or double, so the 1.3 deprecation line was wrong and is gone.

[comphead]

These 27 assertions pin write_java_float_string, which lives in spark-expr, from core -- and most of them already have Spark-verified coverage:

• numeric.rs::test_spark_cast_float_min_value_to_string pins 1.4E-45 and 4.9E-324 for both signs.
• cast_array_to_string.sql:30-31,60-61 pins 3.4028235E38, 1.4E-45, 1.7976931348623157E308, 4.9E-324, NaN, +/-Infinity.
• cast_double_to_string.sql:23-36 pins -0.0, 0.0, +/-1.5, NaN, +/-Infinity, 1.0E20, 0.001.

What is genuinely new is the plain-notation window (9.99E-4, 9999999.0, 1.0E7), f64::MAX, f64::MIN_POSITIVE, and float coverage in general. Those would be better as rows in cast_double_to_string.sql plus a new cast_float_to_string.sql: checkSparkAnswerAndOperator compares against the Spark running in CI rather than against strings transcribed from a JDK, which matters here because the renderer deliberately tracks JDK 19+ shortest-round-trip output while the doc calls it pre-JDK-19.

Then two smoke assertions here are enough to prove the two new match arms are wired.

[andygrove]

Agreed on the SQL file tests. Deferred to #5968 so this fix can land; the core assertions stay until the Spark-checked rows exist.

[comphead]

The same fact -- writing into a String or a string builder cannot fail -- is stated three times: here, at spark_cast_float_to_utf8's let _ =, and in java_float_string in iceberg_partition_path.rs. One statement on the function that returns the fmt::Result is enough; the two let _ = sites can just point at it.

[andygrove]

One statement on write_java_float_string; both let _ = sites point at it.

[comphead]

Minor: the size is justified with an example (-2.2250738585072014E-308, 23 bytes) rather than with the bound. Rust's {:E} emits at most 17 significant digits for an f64, so sign + digit + point + 16 digits + E + sign + 3 digits = 24 bytes worst case. Stating that makes 32 obviously safe and makes the Err arm in write_str obviously unreachable rather than defensively so.

[andygrove]

Comment now states the 24-byte worst case.

[comphead]

Third instance of this shape in the suite: ts_path_native/ts_path_jvm and escaped_native/escaped_jvm also create a table pair, insert identical VALUES, and compare partitionDirs. A helper taking (base name, column DDL, partition spec, VALUES, expected dirs) would collapse all three.

Two other things:

1. Is PARTITIONED BY (f, d) accepted on every Iceberg version the suite runs against? Float/double partitioning has been deprecated since 1.3, and the neighbouring path-spelling test gates its JVM comparison on icebergVersionAtLeast(1, 8). If an older or newer profile rejects the CREATE TABLE, this fails for an unrelated reason.
2. Unlike the other two path tests, this one does not read the rows back through both readers -- assertNativeWriteEngages only checks id. Since the bug was that the directory could not be created at all, a Seq("true", "false").foreach { cometEnabled => ... } readback of f/d would confirm a d=4.9E-324 directory is openable by both.

[andygrove]

Helper and readback deferred to #5968. On the version question: Identity.canTransform accepts float and double on 1.8.1 through 1.11.0, so no gate is needed.

[comphead]

Thanks @andygrove

[sunchao]

Review result

No new actionable defects found in head 8a8a1856e156f281784990dd2ad70c69b34648a0, against base f69c4c81b9429e327ea95658530ae4ed4ed19635.

The change fixes oversized Iceberg partition directory names while preserving existing cast behavior. I checked native formatting, Spark integration, cleanup/error handling, Java compatibility, and test coverage.

Validation

• 8,742,448 base/head cast comparisons passed, covering both float widths, both Arrow string offset widths, nulls, slices, signed zero, infinities, subnormals, and notation boundaries. This used the extracted base/head implementations with the locked Arrow 59.3.0 version.
• Reproduced File name too long for extreme doubles with the old strings; directories using the new strings were created successfully, for both signs.
• 32 Iceberg Java path checks passed across Iceberg 1.10/1.11 and JDK 17/21.
• Confirmed both native writer modes use the formatter and the new test verifies native execution.

Compatibility limit: Java spelling parity remains incomplete, including on JDK 21. For example, double bits 0x2 produce 1.0E-323 versus Java's 9.9E-324. These differences already exist in the cast implementation. All 527,845 sampled outputs round-tripped correctly through JDK 21 (excluding NaN payload preservation), and Iceberg retains typed partition values separately.

CI and testing limits

The native CI job passed 1,453 tests, with 5 skipped, including both new float/double partition-path tests. CI used merge commit e5ae217de0791cb95dadd8173850457756d5dad2; all four PR-changed files match the reviewed head, while other merged code differs. Spark and Iceberg test jobs are still unfinished, with no failures reported at publication time. The new Scala regression test is not yet confirmed passing.

Local full-suite testing was blocked before compilation because the dependency mirror lacks DataFusion 55.1.0. The local results above are component checks, not full Spark/JNI execution.

The existing test follow-ups are already tracked in #5968. No additional review comments.