zhangfengcdt commented on PR #3327: URL: https://github.com/apache/iceberg-rust/pull/3327#issuecomment-5997520069
> Verified against iceberg-java: `Comparators.forType` uses natural order for `float` and `double`, and `Literals.BaseLiteral.equals` compares through that comparator. I also compared `float_cmp` with a transcription of the JDK's `Float.compare` and `Double.compare` on NaN and signed zero edge cases plus 200M random bit patterns and found no difference. > > Nits > > * The spec states this rule too, so the doc on `PrimitiveLiteral` can cite it next to Java. `format/spec.md`, Scan Planning, note on partition equality: "Floating point partition values are considered equal if their IEEE 754 floating-point "single format" bit layout are equal with NaNs normalized to have only the most significant mantissa bit set". > * The doc on `variant_index` restates its name. The useful thing to say is the invariant: it must follow declaration order, because it reproduces the derived `PartialOrd`. > * `test_primitive_literal_order_across_variants` pins the derived cross-variant order, including `AboveMax < BelowMin`. As far as I can tell nothing orders literals of different variants (`strict_metrics_evaluator.rs:307` compares a bound and a literal of the same variant). Keeping `assert_ne!(Int(1), Long(1))` and `assert_eq!(AboveMax, AboveMax)` would cover the `eq` fallback without pinning that order. > > Question > > * The description says `Datum` equality now lines up with its ordering. That holds for signed zero. `Datum::float(f32::NAN) == Datum::float(-f32::NAN)` is still true, while `Datum::partial_cmp` returns `Some(Greater)` through `total_cmp` (`iceberg_float_cmp_f32` in `datum.rs`). This predates the PR, because `OrderedFloat` already made all NaNs equal. Is a sentence on `float_cmp` worth adding to say `Datum` ordering keeps IEEE totalOrder, or is NaN out of scope here? Thanks for the review @comphead ! I have addressed the two inline comments and for the issues listed here, I adjusted the doc order and they should be follow the spec rule now. I changed the description to say it now matches ordering for signed zero. For the NaN case, I'd keep it out of scope here and can open a follow-up issue if you'd like. -- 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]
