andygrove commented on PR #4982:
URL:
https://github.com/apache/datafusion-comet/pull/4982#issuecomment-5059344375
_Review assisted by an LLM (Claude); a human reviewed the findings before
posting._
Thanks for addressing the earlier round. I confirmed all four are resolved:
the stale `planner.rs` banner and leftover docstring are gone, byte/short widen
to `int_val`, the nested-column invariant is documented, and `binary` is in the
page-index gate with a good explanation of the UTF-8 column-index decode. The
dedup design reads well, and the new `residual pool reflects whether the column
type is pushable` test is the right addition since the correctness fuzz test
structurally cannot observe pushdown.
One new thing stood out on a closer read of the Rust side.
### `And`/`Not` in `iceberg_predicate_to_predicate` is unsafe under negation
At `native/core/src/execution/planner.rs:4150-4169`, the Scala
`icebergExprToProto` is deliberately whole-or-nothing on `And`: if either
conjunct fails to convert, it elides the entire residual, and the comment
explains why. Dropping a conjunct is safe in positive position but strengthens
the predicate under a `NOT` via De Morgan (`NOT(A AND B)` = `NOT A OR NOT B`),
which would wrongly prune. The Rust `Node::And` does the opposite. It keeps the
surviving side when one is `None`, with the comment "Dropping a conjunct only
weakens the pruning predicate, so a missing side is safe." That is only true in
positive position. Since `Node::Not` just negates its child, `Not(And(Some,
None))` evaluates to `!p` rather than `!a OR !b`, which is strictly stronger
and would prune row groups that contain matching rows.
This is not a live bug today. It is guarded by the Scala side never emitting
a partial `And` and by `iceberg_literal_to_datum` never returning `None` for a
value the serde actually produces. And these `Not(And(...))` shapes do reach
Rust, since the `NOT` is only pushed to negation-normal form by `rewrite_not()`
after decode, not in Scala. The concern is that the two sides evolve
independently and the Rust code carries an unsafe invariant with a comment
asserting it is safe. If a future change adds an `IcebergLiteral` variant on
the Scala serialize path before the matching `iceberg_literal_to_datum` arm
exists, a residual shaped `Not(A AND newtype_leaf)` would silently mis-prune.
Since the Rust `And`-drop branch provides no benefit today (Scala guarantees
both children are present), mirroring Scala's whole-or-nothing in Rust would be
a strictly safer no-op that removes the footgun:
```rust
match (left, right) {
(Some(l), Some(r)) => Some(l.and(r)),
_ => None,
}
```
At minimum the comment is worth correcting to note the positive-position
assumption and why it holds.
If it is cheap, a residual-level correctness test for `NOT(a AND b)` with
both leaves pushable would exercise this exact shape. The existing `NOT over a
partially supported AND does not drop rows` test covers the elision path but
not the both-convert-under-`NOT` path.
None of this blocks the PR. The overflow fix is correct and the
instrumentation-driven diagnosis was great to follow.
--
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]
