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: [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]
