laskoviymishka commented on code in PR #3416:
URL: https://github.com/apache/iceberg-rust/pull/3416#discussion_r4252387378
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -975,6 +975,88 @@ mod tests {
)
}
+ /// A `FileScanTask` built or deserialized outside `TableScan` carries an
+ /// un-normalized predicate, so a raw `Not(...)` can reach the reader.
Driving
+ /// it through a full read with row-group filtering and row selection
enabled
+ /// exercises both the row-group and page-index evaluators, which negate
NOT
Review Comment:
The doc here says the test exercises both the row-group and page-index
evaluators, but with a single row group and default writer props the row-group
stage prunes first and the page-index path almost certainly never runs — there
may be no column/offset index written at all. So of the two failures this
fixes, only the row-group mispruning is actually under test.
Before merge I'd want a second read test that forces the page-index path —
`row_group_filtering_enabled(false)`, `row_selection_enabled(true)`, a small
`data_page_row_count_limit` so there are real data pages, and an assertion the
file carries a column index. Run against pre-fix code it should fail with the
NOT-unsupported error.
While you're in here, a couple more NOT shapes beyond `NOT(x > 5)` would be
worth it — the ones where the negation isn't a simple flip: `NOT(a AND
b)`/`NOT(a OR b)`, `NOT(x IS NULL)`, `NOT(StartsWith)`, and a NOT arriving via
the equality-delete combination.
##########
crates/iceberg/src/expr/visitors/row_group_metrics_evaluator.rs:
##########
@@ -591,6 +597,30 @@ mod tests {
Ok(())
}
+ #[test]
+ fn eval_error_for_not_predicate_requires_rewrite() -> Result<()> {
+ // The evaluator cannot soundly negate a conservative bound, so a NOT
node
+ // that reaches it (one that was not rewritten away first) is rejected
+ // rather than silently mispruning a matching row group.
+ let row_group_metadata = create_row_group_metadata(1, 1, None, 1,
None)?;
+
+ let (iceberg_schema_ref, field_id_map) =
build_iceberg_schema_and_field_map()?;
+
+ let filter =
(!Reference::new("col_float").greater_than(Datum::float(5.0_f32)))
+ .bind(iceberg_schema_ref.clone(), false)?;
+
+ let result = RowGroupMetricsEvaluator::eval(
+ &filter,
+ &row_group_metadata,
+ &field_id_map,
+ iceberg_schema_ref.as_ref(),
+ );
+
+ assert!(result.is_err());
Review Comment:
`is_err()` on its own can pass for the wrong reason — a field-id lookup or
bind failure would satisfy it just as well, so this doesn't actually prove the
NOT path is what got rejected. I'd pin it:
```rust
let err = result.unwrap_err();
assert_eq!(err.kind(), ErrorKind::Unexpected);
assert!(err.message().contains("NOT"));
```
--
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]
