mbutrovich commented on PR #4982: URL: https://github.com/apache/datafusion-comet/pull/4982#issuecomment-5059932769
Thanks @andygrove, good catch on the asymmetry. You are right that it is not a live bug, and I want to lay out why we are not exposed today, then add a guard so a future change cannot quietly break it. Why we are not exposed today: a partial `And` (one child `Some`, one `None`) is the only shape that makes the old Rust arm wrong, and it cannot arise. The Scala `icebergExprToProto` is whole-or-nothing: it emits an `And` node only when both conjuncts convert, otherwise it returns `None` for the whole node. Rust then decodes every node the Scala side emits, so for any residual that actually reaches `iceberg_predicate_to_predicate`, both children of an `And` convert back to `Some`. The `(Some, None)` branch was dead code. As you note, `Not(And(..))` shapes do reach Rust unchanged, since the negation is only pushed to negation-normal form by `rewrite_not()` after decode, not in Scala. I confirmed the `NOT` survives that far: Iceberg's `Binder.bind` uses only a `BindVisitor` and does not call `rewriteNot`, and `Expressions.not()` only collapses double-negation, it never applies De Morgan. So the correctness of the old code rested entirely on the Scala whole-or-nothing invariant plus S cala/Rust decode parity, which is exactly the coupling you flagged as fragile. What changed: I mirrored Scala's whole-or-nothing in Rust so it is correct even in a release build if the two sides ever diverge, and added a `debug_assert!` tripwire so a divergence fails loudly in dev and CI rather than silently mis-pruning. Both `And` and `Or` now route through one `combine_logical` helper that returns `None` unless both children convert, since both carry the same "both children present" invariant. The helper documents that a `None` child means the serialize and decode paths have drifted (for example a new `IcebergLiteral` variant added on the Scala side without a matching `iceberg_literal_to_datum` arm), which is the scenario you described. I also corrected the misleading "a missing side is safe" comment. Test: Added `NOT over a fully supported AND does not drop rows` in `CometIcebergNativeSuite`, the both-convert-under-`NOT` companion to the existing partial-AND elision test. It reads `NOT(a < 200 AND b > 100)` over two pushable int columns with tiny row groups ordered by `a`, so a wrongly-strengthened `a >= 200` would prune groups holding matching rows, and asserts results match Spark. -- 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]
