sunchao commented on code in PR #6847:
URL: https://github.com/apache/datafusion-comet/pull/6847#discussion_r4241016471
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -733,36 +740,42 @@ object CometIcebergNativeScan extends
CometOperatorSerde[CometBatchScanExec] wit
val left = icebergExprToProto(
IcebergReflection.getMethod(exprClass, "left").invoke(icebergExpr),
output,
- pageIndexUnsupportedColumns)
+ pageIndexUnsupportedColumns,
+ allowPartial)
val right = icebergExprToProto(
IcebergReflection.getMethod(exprClass, "right").invoke(icebergExpr),
output,
- pageIndexUnsupportedColumns)
+ pageIndexUnsupportedColumns,
+ allowPartial)
(left, right) match {
- // Push the residual only if it converts whole. Dropping a conjunct is
safe in positive
- // position but strengthens the predicate under a NOT (De Morgan),
which would wrongly
- // prune, and tracking polarity across arbitrary nesting is error
prone. So an
- // unconvertible conjunct elides the whole residual; the post-scan
CometFilter is exact.
case (Some(l), Some(r)) => Some(logicalPredicate(isAnd = true, l, r))
+ // Dropping a positive conjunct weakens the predicate; the post-scan
filter stays exact.
+ // Inside NOT, every descendant must convert exactly, including nested
AND/OR nodes.
+ case (l, r) if allowPartial => l.orElse(r)
Review Comment:
[P1] Keep Spark-incompatible floating-point comparisons out of partial
residuals. With native Iceberg scanning enabled, create an unpartitioned table
containing `(CAST('-0.0' AS DOUBLE), array(1))` and `(CAST('0.0' AS DOUBLE),
array(1))` as `(f, l)`, then query `WHERE l IS NOT NULL AND f = CAST(0 AS
DOUBLE)`. Spark returns both rows. Previously the unsupported array null check
caused the entire residual to be omitted. This branch now retains `NotNull(f)
AND Eq(f, 0.0)`, and the pinned iceberg-rust reader uses Arrow equality, which
distinguishes signed zeros and drops the negative-zero row. The exact post-scan
filter cannot recover it. Although standalone floating-point pushdown already
has this limitation, this PR exposes it on mixed predicates that previously
avoided it. Please decline unsafe floating-point leaves or provide
Spark-compatible reader comparisons, with a regression covering this mixed
predicate.
Evidence: Fresh reproduction in `/tmp/comet6847-03cb-check.i0elyH`:
`serde-validation.log` shows the actual residual shape converting to `None` at
the base and `AND(NotNull(f), Eq(f, 0.0))` at the head. `spark-zero.log`
records Spark 4.1.3/Iceberg 1.11.0 returning both zero bit patterns.
`native-zero.log` records the same Parquet file and full projected schema
returning two rows without pushdown and one row (`0000000000000000`) with that
predicate. The reader was compiled with iceberg-rust
`af1da4c5bc86178c38c1c6db0578bdd9fb7021e3`, Arrow 59.3.0, and
`with_row_selection_enabled(true)`, matching the checkout. Its
`predicate_visitor.rs::eq` calls `arrow_ord::cmp::eq`; Spark’s
`SQLOrderingUtil.compareDoubles` equates signed zeros across all supported
versions.
--
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]
