andygrove opened a new issue, #5968: URL: https://github.com/apache/datafusion-comet/issues/5968
### What is the problem the feature request solves? Follow-on from the review of #5840 (https://github.com/apache/datafusion-comet/pull/5840#pullrequestreview-5212735173). Two test-placement suggestions were deferred from that PR because they restructure JVM tests rather than change behaviour. **1. Java float rendering is pinned from `core` instead of against Spark.** `native/core/src/execution/operators/iceberg_partition_path.rs` carries 27 assertions (`renders_doubles_like_java_double_to_string`, `renders_floats_like_java_float_to_string`) that pin `write_java_float_string`, which lives in `spark-expr`. Most of them duplicate coverage that already runs against Spark: - `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` pins `3.4028235E38`, `1.4E-45`, `1.7976931348623157E308`, `4.9E-324`, `NaN`, `+/-Infinity`. - `cast_double_to_string.sql` pins `-0.0`, `0.0`, `+/-1.5`, `NaN`, `+/-Infinity`, `1.0E20`, `0.001`. What is genuinely new there is the plain-notation window boundaries (`9.99E-4`, `9999999.0`, `1.0E7`), `f64::MAX`, `f64::MIN_POSITIVE`, and float coverage in general. SQL file tests compare against the Spark running in CI rather than against strings transcribed from a JDK, which matters because the renderer produces JDK 19+ shortest-round-trip digits while CI runs on JDK 17. **2. The Iceberg partition-path Scala tests repeat the same shape three times.** In `CometIcebergWriteActionSuite`, `ts_path_native`/`ts_path_jvm`, `escaped_native`/`escaped_jvm` and `float_path_native`/`float_path_jvm` each create a table pair, insert identical `VALUES`, and compare `partitionDirs`. The float test also does not read the rows back through both readers, unlike the other two, even though the original bug was that the directory could not be created at all. ### Describe the potential solution 1. Add `spark/src/test/resources/sql-tests/expressions/cast/cast_float_to_string.sql` and extend `cast_double_to_string.sql` with rows for the plain-notation window boundaries, `f64::MAX` and `f64::MIN_POSITIVE`. Then trim the two `core` unit tests to a couple of smoke assertions that prove the `Float` and `Double` match arms in `human_string` are wired. 2. Add a helper to `CometIcebergWriteActionSuite` taking (base name, column DDL, partition spec, `VALUES`, expected dirs) and collapse the three table-pair tests onto it. Give the float test a `Seq("true", "false").foreach { cometEnabled => ... }` readback of `f` and `d` so a `d=4.9E-324` directory is shown to be openable by both readers. No version gate is needed for the float test: `Identity.canTransform` accepts float and double on every Iceberg version the suite runs against (1.8.1 through 1.11.0; 1.11 excludes only variant, geometry and geography). It is `bucket` that never accepted them. ### Additional context The Rust-side suggestions from the same review (sealed trait, value const for the smallest subnormal, single infallibility note, buffer size bound) were applied in #5840 itself. -- 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]
