mbutrovich commented on code in PR #3302:
URL: https://github.com/apache/iceberg-rust/pull/3302#discussion_r4298002871
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -836,4 +1073,248 @@ mod tests {
);
assert!(!result.is_null(2));
}
+
+ /// Returns the residual of `predicate` for a data file that stores only
field 1.
+ fn residual_for_file_with_field_1(
+ schema: &SchemaRef,
+ predicate: Predicate,
+ partition: Option<(&PartitionSpec, &Struct)>,
+ ) -> BoundPredicate {
+ let predicate = predicate.bind(schema.clone(), true).unwrap();
+ let mut collector = CollectFieldIdVisitor {
+ field_ids: HashSet::default(),
+ };
+ visit(&mut collector, &predicate).unwrap();
+ residual_for_missing_fields(
+ predicate,
+ &collector.field_ids(),
+ &HashMap::from([(1, 0)]),
+ schema,
+ partition.map(|(spec, _)| spec),
+ partition.map(|(_, data)| data),
+ )
+ .unwrap()
+ }
+
+ /// Asserts that each predicate in `always_true` has the residual
`AlwaysTrue` and each in
+ /// `always_false` has `AlwaysFalse`, for a file that doesn't store the
field `field`.
+ fn assert_residuals_for_missing_field_with_initial_default(
+ field: NestedField,
+ always_true: Vec<Predicate>,
+ always_false: Vec<Predicate>,
+ ) {
+ let schema = Arc::new(
+ Schema::builder()
+ .with_fields(vec![
+ NestedField::required(1, "id",
Type::Primitive(PrimitiveType::Int)).into(),
+ field.into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+
+ let cases = always_true
+ .into_iter()
+ .map(|predicate| (predicate, BoundPredicate::AlwaysTrue))
+ .chain(
+ always_false
+ .into_iter()
+ .map(|predicate| (predicate, BoundPredicate::AlwaysFalse)),
+ );
+ for (predicate, expected) in cases {
+ let message = predicate.to_string();
+ let residual = residual_for_file_with_field_1(&schema, predicate,
None);
+ assert_eq!(residual, expected, "{message}");
+ }
+ }
+
+ /// Mirrors Java's
`TestMetricsRowGroupFilter.testColumnNotInFileWithInitialDefault`.
+ #[test]
+ fn test_residual_for_missing_string_field_with_initial_default() {
+ let country = || Reference::new("country");
+ let string = Datum::string;
+ assert_residuals_for_missing_field_with_initial_default(
+ NestedField::optional(99, "country",
Type::Primitive(PrimitiveType::String))
+ .with_initial_default(Literal::string("US")),
+ vec![
+ country().equal_to(string("US")),
+ country().is_not_null(),
+ country().not_equal_to(string("CA")),
+ country().is_in([string("US"), string("MX")]),
+ country().is_not_in([string("CA"), string("MX")]),
+ country().greater_than_or_equal_to(string("US")),
+ country().less_than(string("ZW")),
+ country().starts_with(string("U")),
+ country().not_starts_with(string("X")),
+ ],
+ vec![
+ country().equal_to(string("CA")),
+ country().is_null(),
+ country().not_equal_to(string("US")),
+ country().is_in([string("CA"), string("MX")]),
+ country().is_not_in([string("US"), string("MX")]),
+ country().less_than(string("AD")),
+ country().greater_than(string("US")),
+ country().starts_with(string("X")),
+ country().not_starts_with(string("U")),
+ ],
+ );
+ }
+
+ /// Mirrors Java's
`TestMetricsRowGroupFilter.testDateColumnNotInFileWithInitialDefault`.
+ #[test]
+ fn test_residual_for_missing_date_field_with_initial_default() {
Review Comment:
> This test looks redundant. The two mutations that failed it (`less_than`
and `greater_than` returning the leaf unchanged) also fail
`test_residual_for_missing_string_field_with_initial_default`. The date literal
conversion happens in `ExpressionEvaluatorVisitor`, not in the new code, so the
test adds no path. Consider dropping it.
I dropped it. I kept the double test for `is_nan`.
--
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]
