You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/09/10 17:53:31 UTC
[PR] fix: render float and double Iceberg partition values like iceberg-java [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: render float and double Iceberg partition values like iceberg-java [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5840:
URL: https://github.com/apache/datafusion-comet/pull/5840
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: render float and double Iceberg partition values like iceberg-java [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #5840:
URL: https://github.com/apache/datafusion-comet/pull/5840#discussion_r4017815957
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
Review Comment:
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:
```rust
/// `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.
##########
native/spark-expr/src/conversion_funcs/mod.rs:
##########
@@ -23,4 +23,5 @@ mod temporal;
pub(crate) mod trim;
mod utils;
+pub use numeric::{write_java_float_string, JavaFloatString};
Review Comment:
`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.
##########
native/core/src/execution/operators/iceberg_partition_path.rs:
##########
@@ -170,7 +170,8 @@ fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal
// `year`/`month`/`day`/`hour` render the ordinal itself and never see a timestamp or binary
// field type (their result types are `int` and `date`), so they cannot collide with the arms
- // below. iceberg-rust already mirrors `TransformUtil` for them.
+ // below. iceberg-rust already mirrors `TransformUtil` for them. `bucket` and `truncate` reject
Review Comment:
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.
##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1110,6 +1110,42 @@ class CometIcebergWriteActionSuite
}
}
+ // iceberg-java renders a `float` or `double` partition value with `Float.toString` /
+ // `Double.toString`. Rust's `Display` spelled `Double.MAX_VALUE` as 309 digits instead, past the
+ // 255-byte limit on one path component (apache/datafusion-comet#5836).
+ test("native acceleration: float and double partition paths match iceberg-java") {
Review Comment:
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.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
+}
- Ok(Arc::new(builder.finish()))
+impl JavaFloatString for f32 {
+ const MIN_SUBNORMAL: &'static str = "1.4E-45";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+impl JavaFloatString for f64 {
+ const MIN_SUBNORMAL: &'static str = "4.9E-324";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+/// Writes `value` as Java's `Float.toString` / `Double.toString` renders it.
+///
+/// If the absolute value is less than 10,000,000 and greater or equal than 0.001, the result is
+/// expressed without scientific notation with at least one digit on either side of the decimal
+/// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having
+/// an optional leading minus sign followed by one digit to the left of the decimal point and the
+/// minimal number of digits greater than zero to the right.
+/// Source: <https://docs.databricks.com/en/sql/language-manual/functions/cast.html>
+///
+/// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's
+/// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would
+/// render as 309 digits. Both matter beyond cosmetics: Spark spells a `cast(double as string)`
+/// this way, and iceberg-java spells a float or double partition directory this way, where the
+/// unabbreviated form overruns the filesystem's limit on one path component.
+///
+/// This is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value.
+/// Only the smallest subnormal, by far the most visible case, is corrected for here.
+///
+/// Errors only if `out` does; writing into a `String` or an arrow string builder cannot fail.
+pub fn write_java_float_string<T: JavaFloatString, W: fmt::Write>(
+ value: T,
+ out: &mut W,
+) -> fmt::Result {
+ let abs = value.abs();
+ if (T::PLAIN_LOWER..T::PLAIN_UPPER).contains(&abs) || abs.is_zero() {
+ write!(out, "{value}")?;
+ if value.fract().is_zero() {
+ // Java always renders a fractional digit; Rust omits it.
+ out.write_str(".0")?;
+ }
+ Ok(())
+ } else if !value.is_finite() {
+ // NaN and the infinities are excluded by the range check above.
+ out.write_str(if value.is_nan() {
+ "NaN"
+ } else if value.is_sign_negative() {
+ "-Infinity"
+ } else {
+ "Infinity"
+ })
+ } else if value.is_smallest_subnormal() {
+ if value.is_sign_negative() {
+ out.write_str("-")?;
+ }
+ out.write_str(T::MIN_SUBNORMAL)
+ } else {
+ // The coefficient has to be inspected before any of it is emitted, so it is formatted
+ // into a stack buffer rather than into `out`, which may not be rewindable.
+ let mut scratch = ExponentBuf::default();
+ write!(scratch, "{value:E}")?;
+ let text = scratch.as_str();
+ match text.split_once('E') {
+ Some((coefficient, exponent)) if !coefficient.contains('.') => {
+ // Java keeps the fractional digit Rust drops from a whole coefficient.
+ out.write_str(coefficient)?;
+ out.write_str(".0E")?;
+ out.write_str(exponent)
}
+ _ => out.write_str(text),
+ }
+ }
+}
- cast::<$offset_type>($from, $eval_mode)
- }};
+/// Scratch space for one `{:E}` rendering, sized past the longest a float can produce
Review Comment:
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.
##########
native/core/src/execution/operators/iceberg_partition_path.rs:
##########
@@ -388,6 +402,60 @@ mod tests {
assert_eq!(civil_from_days(-719_529), (-1, 12, 31));
}
+ fn double(value: f64) -> String {
+ human_string(
+ &Transform::Identity,
+ &Type::Primitive(PrimitiveType::Double),
+ Some(&Literal::Primitive(PrimitiveLiteral::Double(value.into()))),
+ )
+ }
+
+ fn float(value: f32) -> String {
+ human_string(
+ &Transform::Identity,
+ &Type::Primitive(PrimitiveType::Float),
+ Some(&Literal::Primitive(PrimitiveLiteral::Float(value.into()))),
+ )
+ }
+
+ // Expectations taken from `Double.toString` / `Float.toString` output on the JDK
+ // (apache/datafusion-comet#5836).
+ #[test]
+ fn renders_doubles_like_java_double_to_string() {
Review Comment:
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.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
+}
- Ok(Arc::new(builder.finish()))
+impl JavaFloatString for f32 {
+ const MIN_SUBNORMAL: &'static str = "1.4E-45";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+impl JavaFloatString for f64 {
+ const MIN_SUBNORMAL: &'static str = "4.9E-324";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+/// Writes `value` as Java's `Float.toString` / `Double.toString` renders it.
+///
+/// If the absolute value is less than 10,000,000 and greater or equal than 0.001, the result is
+/// expressed without scientific notation with at least one digit on either side of the decimal
+/// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having
+/// an optional leading minus sign followed by one digit to the left of the decimal point and the
+/// minimal number of digits greater than zero to the right.
+/// Source: <https://docs.databricks.com/en/sql/language-manual/functions/cast.html>
+///
+/// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's
+/// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would
+/// render as 309 digits. Both matter beyond cosmetics: Spark spells a `cast(double as string)`
+/// this way, and iceberg-java spells a float or double partition directory this way, where the
+/// unabbreviated form overruns the filesystem's limit on one path component.
+///
+/// This is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value.
+/// Only the smallest subnormal, by far the most visible case, is corrected for here.
+///
+/// Errors only if `out` does; writing into a `String` or an arrow string builder cannot fail.
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: render float and double Iceberg partition values like iceberg-java [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5840:
URL: https://github.com/apache/datafusion-comet/pull/5840#discussion_r4018108971
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
Review Comment:
Done. `MIN_SUBNORMAL: Self` via `from_bits`, spelling moved to `MIN_SUBNORMAL_TEXT`, method removed.
##########
native/spark-expr/src/conversion_funcs/mod.rs:
##########
@@ -23,4 +23,5 @@ mod temporal;
pub(crate) mod trim;
mod utils;
+pub use numeric::{write_java_float_string, JavaFloatString};
Review Comment:
Sealed with a private supertrait and added the owned `java_float_string` next to the writer; core now imports only that.
##########
native/core/src/execution/operators/iceberg_partition_path.rs:
##########
@@ -170,7 +170,8 @@ fn human_string(transform: &Transform, field_type: &Type, value: Option<&Literal
// `year`/`month`/`day`/`hour` render the ordinal itself and never see a timestamp or binary
// field type (their result types are `int` and `date`), so they cannot collide with the arms
- // below. iceberg-rust already mirrors `TransformUtil` for them.
+ // below. iceberg-rust already mirrors `TransformUtil` for them. `bucket` and `truncate` reject
Review Comment:
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.
##########
native/core/src/execution/operators/iceberg_partition_path.rs:
##########
@@ -388,6 +402,60 @@ mod tests {
assert_eq!(civil_from_days(-719_529), (-1, 12, 31));
}
+ fn double(value: f64) -> String {
+ human_string(
+ &Transform::Identity,
+ &Type::Primitive(PrimitiveType::Double),
+ Some(&Literal::Primitive(PrimitiveLiteral::Double(value.into()))),
+ )
+ }
+
+ fn float(value: f32) -> String {
+ human_string(
+ &Transform::Identity,
+ &Type::Primitive(PrimitiveType::Float),
+ Some(&Literal::Primitive(PrimitiveLiteral::Float(value.into()))),
+ )
+ }
+
+ // Expectations taken from `Double.toString` / `Float.toString` output on the JDK
+ // (apache/datafusion-comet#5836).
+ #[test]
+ fn renders_doubles_like_java_double_to_string() {
Review Comment:
Agreed on the SQL file tests. Deferred to #5968 so this fix can land; the core assertions stay until the Spark-checked rows exist.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
+}
- Ok(Arc::new(builder.finish()))
+impl JavaFloatString for f32 {
+ const MIN_SUBNORMAL: &'static str = "1.4E-45";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+impl JavaFloatString for f64 {
+ const MIN_SUBNORMAL: &'static str = "4.9E-324";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+/// Writes `value` as Java's `Float.toString` / `Double.toString` renders it.
+///
+/// If the absolute value is less than 10,000,000 and greater or equal than 0.001, the result is
+/// expressed without scientific notation with at least one digit on either side of the decimal
+/// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having
+/// an optional leading minus sign followed by one digit to the left of the decimal point and the
+/// minimal number of digits greater than zero to the right.
+/// Source: <https://docs.databricks.com/en/sql/language-manual/functions/cast.html>
+///
+/// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's
+/// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would
+/// render as 309 digits. Both matter beyond cosmetics: Spark spells a `cast(double as string)`
+/// this way, and iceberg-java spells a float or double partition directory this way, where the
+/// unabbreviated form overruns the filesystem's limit on one path component.
+///
+/// This is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value.
+/// Only the smallest subnormal, by far the most visible case, is corrected for here.
+///
+/// Errors only if `out` does; writing into a `String` or an arrow string builder cannot fail.
Review Comment:
One statement on `write_java_float_string`; both `let _ =` sites point at it.
##########
native/spark-expr/src/conversion_funcs/numeric.rs:
##########
@@ -136,95 +137,159 @@ macro_rules! cast_float_to_timestamp_impl {
}};
}
-macro_rules! cast_float_to_string {
- ($from:expr, $eval_mode:expr, $type:ty, $output_type:ty, $offset_type:ty, $min_value:expr) => {{
-
- fn cast<OffsetSize>(
- from: &dyn Array,
- _eval_mode: EvalMode,
- ) -> SparkResult<ArrayRef>
- where
- OffsetSize: OffsetSizeTrait, {
- use std::fmt::Write;
-
- let array = from.as_any().downcast_ref::<$output_type>().unwrap();
-
- // If the absolute number is less than 10,000,000 and greater or equal than 0.001, the
- // result is expressed without scientific notation with at least one digit on either side of
- // the decimal point. Otherwise, Spark uses a mantissa followed by E and an
- // exponent. The mantissa has an optional leading minus sign followed by one digit to the
- // left of the decimal point, and the minimal number of digits greater than zero to the
- // right. The exponent has and optional leading minus sign.
- // source: https://docs.databricks.com/en/sql/language-manual/functions/cast.html
-
- const LOWER_SCIENTIFIC_BOUND: $type = 0.001;
- const UPPER_SCIENTIFIC_BOUND: $type = 10000000.0;
-
- // Values are formatted straight into the builder, so no intermediate String
- // is allocated per row. Capacity hint matches arrow-rs's own AVERAGE_STRING_LENGTH
- // (16 bytes / value) so typical fractional and scientific outputs like
- // "1234.5678" or "-1.4E-45" do not force a mid-loop grow.
- let mut builder = GenericStringBuilder::<OffsetSize>::with_capacity(
- array.len(),
- array.len() * 16,
- );
- // Reused across rows by the scientific-notation path, which has to inspect
- // the formatted text before emitting it.
- let mut scratch = String::with_capacity(32);
-
- for value in array.iter() {
- let Some(value) = value else {
- builder.append_null();
- continue;
- };
- let abs = value.abs();
- if (LOWER_SCIENTIFIC_BOUND..UPPER_SCIENTIFIC_BOUND).contains(&abs)
- || abs == 0.0
- {
- let _ = write!(builder, "{value}");
- if value.fract() == 0.0 {
- // Spark always renders a fractional digit; Rust omits it.
- let _ = builder.write_str(".0");
- }
- builder.append_value("");
- } else if !value.is_finite() {
- // NaN and the infinities are excluded by the range check above.
- builder.append_value(if value.is_nan() {
- "NaN"
- } else if value.is_sign_positive() {
- "Infinity"
- } else {
- "-Infinity"
- });
- } else if abs.to_bits() == 1 {
- // Java's Double.toString / Float.toString are not shortest-roundtrip
- // and render the smallest subnormals with more digits than Rust does.
- builder.append_value(if value.is_sign_negative() {
- concat!("-", $min_value)
- } else {
- $min_value
- });
- } else {
- scratch.clear();
- let _ = write!(scratch, "{value:E}");
- match scratch.split_once('E') {
- Some((coefficient, exponent)) if !coefficient.contains('.') => {
- // Spark keeps the fractional digit Rust drops from a whole
- // coefficient.
- let _ = builder.write_str(coefficient);
- let _ = builder.write_str(".0E");
- builder.append_value(exponent);
- }
- _ => builder.append_value(&scratch),
- }
- }
- }
+/// A float width that Java renders through `Float.toString` / `Double.toString`.
+///
+/// `num::Float` supplies the arithmetic predicates; the two widths differ only in the plain-notation
+/// window's endpoints and in the literal text of the smallest subnormal, which Java's algorithm
+/// spells with more digits than a shortest-round-trip formatter produces.
+pub trait JavaFloatString: Float + fmt::Display + fmt::UpperExp {
+ /// `Float.MIN_VALUE` / `Double.MIN_VALUE` as Java spells it.
+ const MIN_SUBNORMAL: &'static str;
+ /// Plain notation covers `[0.001, 10^7)`; anything outside it is scientific.
+ const PLAIN_LOWER: Self;
+ const PLAIN_UPPER: Self;
+
+ /// The value one ULP above zero, the one Java does not render shortest. `Float::min_positive_value`
+ /// is the smallest *normal*, so this has no `num` equivalent.
+ fn is_smallest_subnormal(self) -> bool;
+}
- Ok(Arc::new(builder.finish()))
+impl JavaFloatString for f32 {
+ const MIN_SUBNORMAL: &'static str = "1.4E-45";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+impl JavaFloatString for f64 {
+ const MIN_SUBNORMAL: &'static str = "4.9E-324";
+ const PLAIN_LOWER: Self = 0.001;
+ const PLAIN_UPPER: Self = 10000000.0;
+
+ fn is_smallest_subnormal(self) -> bool {
+ self.abs().to_bits() == 1
+ }
+}
+
+/// Writes `value` as Java's `Float.toString` / `Double.toString` renders it.
+///
+/// If the absolute value is less than 10,000,000 and greater or equal than 0.001, the result is
+/// expressed without scientific notation with at least one digit on either side of the decimal
+/// point. Otherwise the value is a mantissa followed by `E` and an exponent, the mantissa having
+/// an optional leading minus sign followed by one digit to the left of the decimal point and the
+/// minimal number of digits greater than zero to the right.
+/// Source: <https://docs.databricks.com/en/sql/language-manual/functions/cast.html>
+///
+/// Rust's own `Display` and `UpperExp` give the same digits but drop a whole coefficient's
+/// fractional zero (`1` for `1.0`) and never switch to an exponent, so `Double.MAX_VALUE` would
+/// render as 309 digits. Both matter beyond cosmetics: Spark spells a `cast(double as string)`
+/// this way, and iceberg-java spells a float or double partition directory this way, where the
+/// unabbreviated form overruns the filesystem's limit on one path component.
+///
+/// This is the pre-JDK-19 `Double.toString`, which is not shortest-round-trip for every value.
+/// Only the smallest subnormal, by far the most visible case, is corrected for here.
+///
+/// Errors only if `out` does; writing into a `String` or an arrow string builder cannot fail.
+pub fn write_java_float_string<T: JavaFloatString, W: fmt::Write>(
+ value: T,
+ out: &mut W,
+) -> fmt::Result {
+ let abs = value.abs();
+ if (T::PLAIN_LOWER..T::PLAIN_UPPER).contains(&abs) || abs.is_zero() {
+ write!(out, "{value}")?;
+ if value.fract().is_zero() {
+ // Java always renders a fractional digit; Rust omits it.
+ out.write_str(".0")?;
+ }
+ Ok(())
+ } else if !value.is_finite() {
+ // NaN and the infinities are excluded by the range check above.
+ out.write_str(if value.is_nan() {
+ "NaN"
+ } else if value.is_sign_negative() {
+ "-Infinity"
+ } else {
+ "Infinity"
+ })
+ } else if value.is_smallest_subnormal() {
+ if value.is_sign_negative() {
+ out.write_str("-")?;
+ }
+ out.write_str(T::MIN_SUBNORMAL)
+ } else {
+ // The coefficient has to be inspected before any of it is emitted, so it is formatted
+ // into a stack buffer rather than into `out`, which may not be rewindable.
+ let mut scratch = ExponentBuf::default();
+ write!(scratch, "{value:E}")?;
+ let text = scratch.as_str();
+ match text.split_once('E') {
+ Some((coefficient, exponent)) if !coefficient.contains('.') => {
+ // Java keeps the fractional digit Rust drops from a whole coefficient.
+ out.write_str(coefficient)?;
+ out.write_str(".0E")?;
+ out.write_str(exponent)
}
+ _ => out.write_str(text),
+ }
+ }
+}
- cast::<$offset_type>($from, $eval_mode)
- }};
+/// Scratch space for one `{:E}` rendering, sized past the longest a float can produce
Review Comment:
Comment now states the 24-byte worst case.
##########
spark/src/test/scala/org/apache/comet/CometIcebergWriteActionSuite.scala:
##########
@@ -1110,6 +1110,42 @@ class CometIcebergWriteActionSuite
}
}
+ // iceberg-java renders a `float` or `double` partition value with `Float.toString` /
+ // `Double.toString`. Rust's `Display` spelled `Double.MAX_VALUE` as 309 digits instead, past the
+ // 255-byte limit on one path component (apache/datafusion-comet#5836).
+ test("native acceleration: float and double partition paths match iceberg-java") {
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org