comphead commented on code in PR #3327:
URL: https://github.com/apache/iceberg-rust/pull/3327#discussion_r4179687433
##########
crates/iceberg/src/spec/values/primitive.rs:
##########
@@ -46,7 +55,60 @@ pub enum PrimitiveLiteral {
BelowMin,
}
+impl PartialEq for PrimitiveLiteral {
+ fn eq(&self, other: &Self) -> bool {
+ self.partial_cmp(other) == Some(Ordering::Equal)
+ }
+}
+
+impl PartialOrd for PrimitiveLiteral {
+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+ match (self, other) {
+ (Self::Boolean(a), Self::Boolean(b)) => a.partial_cmp(b),
+ (Self::Int(a), Self::Int(b)) => a.partial_cmp(b),
+ (Self::Long(a), Self::Long(b)) => a.partial_cmp(b),
+ (Self::Float(a), Self::Float(b)) => Some(float_cmp(a, b)),
+ (Self::Double(a), Self::Double(b)) => Some(float_cmp(a, b)),
+ (Self::String(a), Self::String(b)) => a.partial_cmp(b),
+ (Self::Binary(a), Self::Binary(b)) => a.partial_cmp(b),
+ (Self::Int128(a), Self::Int128(b)) => a.partial_cmp(b),
+ (Self::UInt128(a), Self::UInt128(b)) => a.partial_cmp(b),
+ // Different variants order by declaration, as the derived impl
did.
+ _ => self.variant_index().partial_cmp(&other.variant_index()),
Review Comment:
The derive was exhaustive and this arm is not. If a variant is added, the
compiler only asks for an arm in `variant_index`. Two values of the new variant
then reach `_`, get the same index and compare `Equal`, so `==` is true for any
pair. That would hit `&datum == literal` in `expression_evaluator.rs` and
`current != partition_value` in `ClusteredWriter`. `Datum::partial_cmp` fails
closed here with `_ => None`.
Handling the unit variants explicitly and asserting in the fallback keeps a
safety net in debug builds:
```suggestion
(Self::AboveMax, Self::AboveMax) | (Self::BelowMin,
Self::BelowMin) => {
Some(Ordering::Equal)
}
// Different variants order by declaration, as the derived impl
did.
_ => {
debug_assert_ne!(std::mem::discriminant(self),
std::mem::discriminant(other));
self.variant_index().partial_cmp(&other.variant_index())
}
```
##########
crates/iceberg/src/expr/visitors/expression_evaluator.rs:
##########
@@ -596,6 +596,38 @@ mod tests {
Ok(())
}
+ #[test]
+ fn test_expr_eq_signed_zero() -> Result<()> {
+ let case_sensitive = true;
+ let (partition_spec, schema) =
create_partition_spec(PrimitiveType::Float)?;
+
+ let data_file = DataFile {
+ partition: Struct::from_iter([Some(Literal::float(-0.0_f32))]),
+ ..create_data_file_float()
+ };
+
+ // -0.0 and 0.0 are different values, as in iceberg-java's evaluator.
+ for (literal, expected) in [(0.0_f32, false), (-0.0_f32, true)] {
Review Comment:
The description says `=` and `IN` change, but this test only covers `Eq`.
`IN` is a separate path. By my trace, on the base `Reference::is_in([0.0,
-0.0])` collapses to one literal in the `FnvHashSet` and `bind` then rewrites
it to `Eq` (`predicate.rs:470`), so a set holding both zeros was not possible
before this PR.
Could the test also cover these two cases, in the style of `test_expr_in`
above?
- `a IN (0.0, 1.0)` against the `-0.0` partition should be false. On the
base it is true, because `contains` matches `0.0` through the old `eq`.
- `a IN (0.0, -0.0)` should keep both literals after `bind`, as a
`BoundPredicate::Set` rather than `Binary(Eq)`.
--
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]
