Repository navigation
Conversation
Decline FLOAT and DOUBLE range comparisons and zero or NaN literals. Spark treats -0.0 as equal to 0.0 and sorts every NaN above all numbers, while iceberg-rust compares by IEEE 754 total order, and its row-group pruning trusts statistics that can omit NaN (apache/iceberg-rust#3494). Decline equality under NOT on nullable columns. Iceberg passes `<=>` as `=`, so the native reader dropped NULL rows that Spark keeps for `NOT (id <=> 1)`. On columns whose type changed in the schema history, push only equality outside NOT and null checks, since iceberg-rust compares older, narrower files incorrectly (apache/iceberg-rust#3122). An incomplete history treats every BIGINT, DOUBLE and DECIMAL column as promoted. Closes apache#7136. Closes apache#7137. Part of apache#7138.
github-actions
Bot
added
bug
Spark rewrites `b = false` on a nullable boolean as `b IS NOT NULL AND NOT b`, which Iceberg passes as `not_null(b) AND not(b = true)`. Declining equality under NOT on a nullable column left that residual unpushed, so CometFuzzIcebergSuite found no residual for its boolean column. A NULL value fails a NOT_NULL conjunct, and with it the whole AND, in Spark and in the native reader alike. Treat a column checked that way as non-nullable within the AND, so the null-safe concern no longer applies.
…cans Extract referencedColumn so the residual converter and notNullColumns share the bare-reference check. Share the residual predicate loop of the new native tests, and assert that `id != 1` and `b = false` are still pushed while `NOT (id <=> 1)` and `b IS NOT TRUE` are not. CometFuzzIcebergSuite samples its equality literal from the data. Skip zero as well as NaN, since floating-point comparisons with zero are no longer pushed. Note in the Iceberg guide that `!=` is not affected by the negated-equality rule.
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Each case the serde now declines matches a real gap between Spark and the pinned iceberg-rust, which also installs the residual as a Parquet RowFilter, so a looser residual is not safe here as it is for Iceberg Java. Three points on the tests, a reference and a helper.
| * unpushed. | ||
| */ | ||
| private def checkResidualPredicates(table: String, predicates: Seq[String]): Unit = | ||
| for (predicate <- predicates; prefix <- Seq("", "l IS NOT NULL AND ")) { |
There was a problem hiding this comment.
With the prefix, the residual never converts. The AND branch needs both sides, and the null check on the array column l is declined by isComplexType, so each prefixed query reruns the unprefixed one with nothing pushed. The docstring says as much. That doubles the query count of the three new tests without reaching the code this PR changes. The prefix starts to matter with partial conjuncts in #6847. Could the prefix and the l column move there, so this PR and its backports only run the cases they fix?
There was a problem hiding this comment.
Removed here; #6847 retains the partial-conjunct tests.
| // Spark sorts every NaN above all numbers. The native row filter sorts negative NaNs | ||
| // below them, and iceberg-rust prunes row groups on min/max statistics that can | ||
| // leave NaN out, or hold it as both when parquet-mr 1.9 or earlier starts a row | ||
| // group with NaN (apache/iceberg-rust#3494). |
There was a problem hiding this comment.
apache/iceberg-rust#3494 was closed on 2026-10-23 by apache/iceberg-rust#3502 ("treat NaN row group bounds as might-match for inequalities"), which is 54 commits past the pinned 1f3bc34. Could the comment name #3502, so the next pin bump knows which part of this exclusion it can revisit? The fix is for row-group bounds, so the page index side would still need checking then.
There was a problem hiding this comment.
Referenced #3502 and retained the page-index caveat.
| def promotedColumns(table: Any, schema: Any): Set[String] = { | ||
| import scala.jdk.CollectionConverters._ | ||
| val findField = getMethod(schema.getClass, "findField", classOf[Int]) | ||
| def typeOf(field: Any): String = getMethod(field.getClass, "type").invoke(field).toString |
There was a problem hiding this comment.
fieldTypeOf at line 1416 already does this lookup, so this can be fieldTypeOf(field).toString, the way this method already reuses fieldIdOf.
There was a problem hiding this comment.
Reused fieldTypeOf(field).toString.
|
@comphead could you take another look? thanks! |
andygrove
left a comment
andygrove
left a comment
There was a problem hiding this comment.
Thanks for splitting this out of #6847. I traced the new underNot and AND non-null handling by hand and the polarity reasoning holds. A NOT_NULL conjunct is safe even under a NOT, because rewrite_not turns it into an IsNull disjunct that keeps the NULL row, as Spark does. I also checked the pinned iceberg-rust row_group_metrics_evaluator.rs. Its eq and in already treat NaN row-group bounds as might-match, so leaving equality and IN with non-zero float literals eligible looks sound. The regression tests compare against Spark and cover the signed-zero, null-safe and promotion cases well.
I have a few things I would like addressed before merge.
Incomplete schema history and the v3 date promotion. In IcebergReflection.promotedColumns, promotable only covers long, double and decimal(. The v3 spec also allows date to timestamp and timestamp_ns. When the history is complete, comparing type strings catches it. When the history has a gap, would a TimestampNTZ column still push range and IN predicates against files that store date? Comet pushes TimestampNTZType literals, so this looks like the same bug class as apache/iceberg-rust#3122. Could promotable include timestamp and timestamp_ns, with a case in "an incomplete schema history treats every promotable column as promoted"?
The user guide undersells that rule. iceberg.md says the incomplete-history exclusion applies to "every BIGINT and DOUBLE column", but the code also covers DECIMAL (and the timestamp types if the previous point is accepted). Could the sentence list them?
The cost of the float exclusion. This trades native pruning on FLOAT and DOUBLE range filters for correctness, and I agree with that trade while the page-index and row-filter semantics are unverified. Those filters are common though, and after this change they get no native pruning at all, even on files with no NaN or signed zero. Could the PR description say that these filters now rely on the post-scan filter? Could you also open an issue to restore range pruning once iceberg-rust#3502 plus the page-index and row-filter side are verified, and reference it from the comment in CometIcebergNativeScan.scala? If #5883 already covers it, linking that is fine.
Time travel. promotedColumns is also called with metadata.scanSchema for VERSION AS OF, but only the unit test exercises that path. Could you add a VERSION AS OF read after the INT to BIGINT promotion in "literal residuals on promoted columns keep rows from older files", compared against Spark?
…is incomplete Iceberg format v3 allows promoting a date column to timestamp or timestamp_ns. When snapshot expiration has removed older schemas, such a column can still have data files that store dates, so treat it as promoted, as long, double and decimal columns already are. Add a VERSION AS OF read to the promoted-column regression test. Iceberg cannot plan a filter on a dropped column, so the test drops id and adds it back as a new INT column first. Only the snapshot's schema then shows that the id this read filters on was promoted. Correct the code comment that said the scan schema covers a dropped column. The Iceberg guide now lists DECIMAL and TIMESTAMP_NTZ in the incomplete-history rule, says the native scan does not prune row groups for these predicates, and links apache#7588, which tracks restoring FLOAT and DOUBLE range pruning.
|
@andygrove thanks for the review! Addressed in e1afd07:
|
|
@andygrove when you have a sec, could you add |
Done |
Which issue does this PR close?
Closes #7136.
Closes #7137.
Part of #7138.
Rationale for this change
The native Iceberg reader uses residuals to prune reads and filter rows. Comparisons that differ from Spark can discard matching rows before the post-scan filter runs. These correctness fixes were split from #6847 so they can land and be backported independently.
What changes are included in this PR?
Leave unsafe residuals to the post-scan filter:
!=and IN predicates remain eligible.b = false.long,double,decimal,timestampandtimestamp_nscolumn is treated as promoted.These predicates now rely on the post-scan filter. Iceberg still skips data files by their bounds, but the native scan no longer prunes row groups or pages for them. This matters most for
FLOATandDOUBLErange filters. #7588 tracks restoring that pruning.Unrecorded promotions and imported files remain a known limitation under #7138. The Iceberg guide documents the exclusions.
How are these changes tested?
VERSION AS OFread returns 2 of 6 rows when promoted columns come from the table schema alone. Thetimestampcase fails with the previous list of promotable types. Both pass with the change.Normal compilation, Scalastyle and Spotless passed. Other runtime profiles were not run.