laskoviymishka commented on code in PR #3177:
URL: https://github.com/apache/iceberg-rust/pull/3177#discussion_r3972028887
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -246,14 +246,20 @@ impl PredicateConverter<'_> {
/// Build an Arrow predicate that always returns true.
fn build_always_true(&self) -> Result<Box<PredicateResult>> {
Ok(Box::new(|batch| {
- Ok(BooleanArray::from(vec![true; batch.num_rows()]))
+ Ok(BooleanArray::new(
Review Comment:
Small thing — these two helper sites got written multi-line while the
`r#in`/`not_in` accumulator sites are single-liners, and at this indent they're
only 80 chars, so `cargo fmt` will want to collapse them to match.
Since all four sites now repeat the same `new_set`/`new_unset` idiom, a tiny
private `constant_bool_array(n: usize, value: bool) -> BooleanArray` would
unify them and sidestep the formatting question entirely. wdyt?
##########
crates/iceberg/src/arrow/reader/predicate_visitor.rs:
##########
@@ -590,7 +596,7 @@ impl BoundPredicateVisitor for PredicateConverter<'_> {
// update this if arrow ever adds a native is_in kernel
let left = project_column(&batch, idx)?;
- let mut acc = BooleanArray::from(vec![false;
batch.num_rows()]);
+ let mut acc =
BooleanArray::new(BooleanBuffer::new_unset(batch.num_rows()), None);
Review Comment:
These `is_in`/`not_in` accumulator seeds (and the
`always_true`/`always_false` paths) aren't run against a batch by any test that
asserts the output, so a swapped `new_set`/`new_unset` — or an n=0 regression —
would pass silently.
Since the whole point here is output-identical behavior, I'd add a small
test over a zero-row and a nonzero batch asserting the values are the expected
constant and `null_count()` is zero. Cheap guard for a path that's easy to get
subtly wrong on a future edit.
--
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]
