JeonDaehong commented on code in PR #3502:
URL: https://github.com/apache/iceberg-rust/pull/3502#discussion_r4336967896


##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -1724,6 +1731,60 @@ mod tests {
         Ok(())
     }
 
+    fn eval_nan_bounds_inequality(filter: Predicate) -> Result<bool> {
+        let row_group_metadata = create_row_group_metadata(
+            1,
+            1,
+            Some(Statistics::float(
+                Some(f32::NAN),
+                Some(f32::NAN),
+                None,
+                Some(0),
+                false,
+            )),
+            1,
+            None,
+        )?;
+
+        let (iceberg_schema_ref, field_id_map) = 
build_iceberg_schema_and_field_map()?;
+        let filter = filter.bind(iceberg_schema_ref.clone(), false)?;
+
+        RowGroupMetricsEvaluator::eval(
+            &filter,
+            &row_group_metadata,
+            &field_id_map,
+            iceberg_schema_ref.as_ref(),
+        )
+    }
+
+    #[test]
+    fn eval_true_for_nan_lower_bound_less_than() -> Result<()> {
+        let filter = 
Reference::new("col_float").less_than(Datum::float(5.0_f32));
+        assert!(eval_nan_bounds_inequality(filter)?);
+        Ok(())
+    }
+
+    #[test]
+    fn eval_true_for_nan_lower_bound_less_than_or_eq() -> Result<()> {
+        let filter = 
Reference::new("col_float").less_than_or_equal_to(Datum::float(5.0_f32));
+        assert!(eval_nan_bounds_inequality(filter)?);
+        Ok(())
+    }
+
+    #[test]
+    fn eval_true_for_nan_upper_bound_greater_than() -> Result<()> {

Review Comment:
   These pass on main too, since a positive NaN max is above `5.0` under 
`total_cmp`. A NaN with the sign bit set sorts below `-Inf` instead. With min = 
max = `f32::from_bits(0xffc0_0000)`, main prunes the row group for `f > 5.0` 
and `f >= 5.0`, and this PR keeps it. I checked by dropping the new `is_nan()` 
block. Running these two tests (or all four) with that bound as well would pin 
the upper-bound half of the fix.



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

Reply via email to