Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full four-file diff from bfacc43a50aba13da4fa8a828db52309541191b4 to 89d073b16f34bf7e47fab5ffd5129d9089e95e2f. The PR is open and not a draft. No introduced P1/P2 issues found within this review.
Applied review-comet-pr and review-comet-expression-pr for the residual serializer change. Read AGENTS.md and relevant contributor guidance. The snapshot and live GitHub discussion contained no reviews, conversation comments, inline comments, or review threads. No existing blockers were identified.
Summary
- Prior state and problem: An unsupported conjunct, such as an array null check, previously discarded the entire Iceberg residual, losing primitive predicate pruning while retaining correct post-scan filtering.
- Design approach: Preserve supported positive conjuncts. Require safe predicates from both
ORbranches and exact conversion throughout aNOTchild. - Correctness: The changed recursion preserves the requirement that pushed predicates cannot exclude qualifying rows. Checked Spark’s
And,Or, andNotevaluation and code generation, including null semantics, against supported-version sources. Traced post-scan filter preservation and native predicate decoding, binding, pruning, and row filtering. No P1/P2 correctness issue was identified. - Compatibility analysis: Boolean evaluation is identical across Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Reviewed their filter-pushdown handling and the corresponding Iceberg scan builders. The public serializer signature, protobuf contract, dependencies, and type restrictions remain unchanged.
- Key design decisions:
allowPartial = falsepropagates through every descendant beneathNOT. UnsupportedORbranches still prevent that disjunction from being pushed. Existing complex-type, projection, transform, and page-index restrictions remain effective. - Implementation sketch: A private overload carries the conversion policy through the existing traversal. Tests cover operand order, unsupported leaves, nested logical operators, complex null checks, native execution, Spark result parity, and reader metrics. The Iceberg guide describes the resulting behavior.
- Performance: Exact-head CI passed the single-file, multiple-row-group regression requiring the selective scan to read less than half the baseline bytes, with unchanged split counts. This supports reduced reader work for the tested workload. The change adds no extra tree traversal, and no evidence-backed performance regression was identified.
- Design: The conservative treatment of negation is straightforward to reason about. It enables useful pruning without changing the exact post-scan filter or introducing broader predicate rewriting.
- Abstraction & complexity: One private Boolean parameter is sufficient for the required distinction. The existing public entry point remains intact, and no additional abstraction appears necessary.
- Behavioral changes worth calling out: Compared with
branch-1.1ate9efd9f764ee0a59b7898ff028d6985d4a7a28e1, whose residual converter matches the PR base, mixed supported/unsupported positive predicates now retain pruning. This is an intended, documented efficiency change with unchanged query results. - Suggested improvements: Complete the already-requested
run-iceberg-testsvalidation before entering the merge queue. No additional code change meeting the P1/P2 reporting bar was identified.
Exact-head CI: The latest check per name reports 26 successes and 15 skips, with no failing or pending check runs. Inspected CI run 38032305374 and its logs. All 28 serializer-suite tests passed, including the four new cases. The scans job passed 687 tests, including the new parity/I/O regression, with 15 cancellations, one ignored test, and no failures or aborted suites.
Validation limits: Broader Iceberg-version suites, Spark SQL suites, and macOS testing were skipped. I did not rebuild or rerun Comet locally because this checkout lacks build outputs and the required Maven dependencies. A disposable independent three-valued-logic model checked 22,140 expression trees through seven nodes across 1,793,340 tree/value combinations, finding no unsafe strengthening. That validates the logical policy, not production execution. Production execution evidence comes from the inspected exact-head CI logs. The project working tree remains unchanged.
89d073b to
03cb063
Compare
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit 03cb06391bf5f6e6fcc4bb55a3ae2e2731d7dcf9. New P1/P2 findings are detailed inline.
| 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) |
There was a problem hiding this 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.
There was a problem hiding this comment.
Addressed at aaf808d29: FLOAT/DOUBLE comparisons and IN predicates are no longer pushed, including standalone predicates and leaves under AND/OR/NOT. Floating-point null checks and independent safe conjuncts remain eligible.
Added the mixed signed-zero regression; both the serializer and runtime controls fail before the fix. At this head, 29 serializer tests, six targeted native tests (including 44 floating-point queries) and upstream CI passed.
Could you apply run-iceberg-tests for broader coverage? I tried the fork CI path, but GitHub rejects dispatch with “Actions has been disabled for this repository.”
|
@sunchao thanks for the review , I will update as soon as possible |
andygrove
left a comment
There was a problem hiding this comment.
Thanks @rich7420. The partial-pushdown logic looks right to me: an AND keeps its convertible side, an OR still needs both, and allowPartial = false reaches every descendant under a NOT. Two things:
- Declining every FLOAT/DOUBLE comparison also drops pruning on predicates like
f > 5.0, which 1.1 pushes. As far as I can tell, Arrow's total order and Spark only disagree when the literal is±0.0or NaN. For any other literal,-0.0/0.0compare the same way, and NaN sorts above everything in both. Could the rule decline only those literals, including insideIN, after checking that Parquet's float statistics agree? The signed-zero regression test would still cover the bug. - The standalone float pushdown is a wrong-results bug in 1.1 (
WHERE f = 0.0drops-0.0rows on a native Iceberg scan). Could you open an issue for it and reference it from the PR description, so the fix is tracked and shows up in the changelog?
I've applied run-iceberg-tests.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit aaf808d295992af3badee81691cf0d0e1ce6ec4d. New P1/P2 findings are detailed inline.
| 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) |
There was a problem hiding this comment.
[P1] [P1] Preserve NULL matches in retained negated null-safe comparisons. With native Iceberg scanning enabled, create an unpartitioned (id INT, l ARRAY<INT>) table containing (NULL, array(1)), (1, array(1)), and (2, array(1)), then query WHERE l IS NOT NULL AND NOT (id <=> 1). Spark returns the NULL and 2 rows. The base omits this mixed residual, but this branch now retains NOT(Eq(id, 1)). Native rewrite_not() turns it into NotEq, whose Arrow comparison produces NULL for a null input and filters that row out. Only ID 2 survives, and the post-scan filter cannot recover the missing row. Please decline these null-sensitive negative residuals, including those produced by NOT, or preserve their null-inclusive semantics in native filtering, with a regression for this mixed predicate.
Evidence: Fresh reproduction in /tmp/comet6847-review-20261019-phKbuI: nullsafe-spark.log records Spark 4.1.3/Iceberg 1.11.0 returning [NULL, 2], the actual residual (not_null(l) and not(id == 1)), base serialization None, and exact-head serialization NOT(Eq(id, 1)). nullsafe-native.log records the same Parquet file returning [NULL, 1, 2] without pushdown and [2] with the retained predicate, leaving two versus one rows after the original condition. The reader uses iceberg-rust af1da4c5bc86178c38c1c6db0578bdd9fb7021e3, Arrow 59.3.0, rewrite_not().bind(...), and with_row_selection_enabled(true), matching the checkout. Its predicate_visitor.rs::not_eq calls Arrow neq, which returns null for null input.
There was a problem hiding this comment.
Fixed. Equality on a nullable column is no longer pushed when it appears anywhere under NOT, so l IS NOT NULL AND NOT (id <=> 1) keeps the row where id is NULL. The runtime regression test covers your expression and its nested variants. This fix now lives in #7156.
There was a problem hiding this comment.
@rich7420. Keeping the convertible side of a positive AND while OR and NOT still require an exact child is the right shape: whatever reaches the reader is implied by the residual, and the filter above the scan stays exact. The partial-residual test is a real pruning check, with one file, several row groups, equal splits and the bytes read halved. A few things, building on the open threads rather than repeating them.
On sunchao's NOT (id <=> 1) thread. I traced it and it reproduces the way he describes. Spark's V2ExpressionBuilder turns Not(EqualTo) into <>, but Not(EqualNullSafe) into NOT(<=>). Iceberg's SparkV2Filters maps <=> to EQ, so the residual is NOT(Eq(id, 1)). iceberg-rust's RewriteNotVisitor negates that to NotEq, and predicate_visitor.rs::not_eq calls arrow's neq, which gives NULL for a NULL input, so the row filter drops the row Spark keeps. The serde can't tell = from <=> here because Iceberg collapses both to EQ. The standalone NOT (id <=> 1) already pushes on main and branch-1.1, so that part predates this PR, but this PR extends it to mixed residuals like sunchao's l IS NOT NULL AND NOT (id <=> 1). It also nests: NOT (a <=> 1 AND b > 2) with a NULL and b = 5 is true in Spark and NULL in arrow. Two ways out:
- Decline any
EQleaf under a NOT, at any depth. - Convert an
EQleaf under a NOT asAND(NotNull(col), Eq(col, lit)), which the NOT rewrite turns intoOR(IsNull(col), NotEq(col, lit)). That's exact for<=>, weaker but still correct for=, and it keeps the pruning.
A runtime regression on sunchao's shape, plus a nested one, would pin it.
On narrowing the float decline to ±0.0 and NaN literals. One wrinkle worth checking before narrowing it for ordering predicates: arrow 59.3 compares floats with IEEE total order (total_cmp), where a NaN with its sign bit set sorts below -inf, while Spark orders every NaN greatest. If I'm reading it right, WHERE d > 5.0 would then drop a -NaN row that Spark returns. SELECT -CAST('NaN' AS DOUBLE) produces one, and Parquet keeps the bits. A narrowed rule might need to leave out > and >=, skip NOT (the rewrite turns NOT (d < x) into d >= x), and be checked against the row-group and page evaluators too, so keeping the blanket decline for now and narrowing it in a follow-up may be simpler. The runtime test would only guard it if it seeded a -NaN row and checked d > 1.0 and d >= 1.0. Today it uses +NaN and literals 0, -0.0 and 1, so a later narrowing that re-admits d > 1.0 would still pass at runtime and be caught only by the serde test. The comment at CometIcebergNativeScan.scala:911-913 could name the sign of NaN and signed zeros as the reasons, so the next reader doesn't narrow it that way.
Tests.
isnan(f),f = CAST('NaN' AS DOUBLE)andf != CAST('NaN' AS DOUBLE)reach Iceberg asIS_NAN/NOT_NANand fall throughcase _ => Nonetoday, but nothing pins that.- The OR and NOT shapes in the partial test only check parity. Asserting bytes read for one OR that should prune and one NOT that shouldn't (
NOT (id < 2000 AND l IS NOT NULL)reading the whole file) would check those guards directly. filter pushdown - comparison operatorsand thecategory = 'B' OR value > 30.0case filter on a DOUBLE column, so they no longer push anything. The suite has no plain OR pushdown test on a pushable type left outside the new partial test. Moving those fixtures to BIGINT would keep that coverage.- The bind-failure fallback in
planner.rs(Skipping Iceberg predicate pushdown; bind failed) lost its only test: thef > 1.1case on a FLOAT column reached it through the double-to-float mismatch and no longer sends a literal.
Description. The float decline and the <=> fix both change results that branch-1.1 gets wrong, so the description should link the tracking issues Andy asked for, and iceberg.md's "Reads that differ from Spark" section is the natural place to note them until they're fixed.
It merges cleanly onto current main, and its hunks are separate from #7130's Iceberg scan changes.
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit 453441bd657e5c63549dbcc710c36fe10242ee76. New P1/P2 findings are detailed inline.
| .builder(icebergTable.spec()) | ||
| .withPath(file.getAbsolutePath) | ||
| .withFileSizeInBytes(file.length()) | ||
| .withRecordCount(rows.size) |
There was a problem hiding this comment.
[P2] Convert rows.size to Long explicitly before passing it to withRecordCount. The required Spark 3.5/Scala 2.12 strict build enables numeric-widening warnings as errors, so this new fixture fails test compilation with implicit numeric widening instead of completing successfully. This blocks the required CI check. Using withRecordCount(rows.size.toLong) preserves the fixture's behavior and removes the compilation failure.
Evidence: Exact-head CI job https://github.com/apache/datafusion-comet/actions/runs/39475272780/job/118816221478 ran ./mvnw -B test-compile -Pspark-3.5,delta -Pstrict-warnings -DskipTests and reported CometIcebergNativeSuite.scala:4612: implicit numeric widening, followed by failure of scala-test-compile-first. The changed line passes the Int-valued rows.size to the Long-valued record-count API.
There was a problem hiding this comment.
Fixed with withRecordCount(rows.size.toLong). The Spark 3.5 / Scala 2.12 -Pstrict-warnings test-compile now passes locally.
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for working through all the cases that came up in review.
What does "native Iceberg pruning" in the description refer to? By the time Comet serializes a FileScanTask, Iceberg planning has already pruned files and partitions. What reaches the reader is the task's residual, the part of the filter that the file's partition values don't settle. At the iceberg-rust rev Comet uses, ArrowReader uses that predicate in three ways: a Parquet RowFilter evaluated while decoding (pipeline.rs), row-group statistics (pipeline.rs), and page index row selection (pipeline.rs), which Comet turns on (iceberg_scan.rs). Could the description name which of these the kept conjuncts are meant to help? They cost different amounts, which leads to the next question.
The title says perf:, but I couldn't find performance numbers in the description or the thread. The closest evidence is the test that checks bytes_scanned halves (CometIcebergNativeSuite.scala). Its fixture is the best case for pruning: one file sorted on id, 16 KiB uncompressed row groups, and a 1% range on id. Could you add before and after numbers from a benchmark? CometIcebergReadBenchmark has no filter case today, so a case there would also stay useful. I'd most like to see the case where the change gains least: a conjunct on a column that isn't clustered, so statistics skip nothing. There the reader still evaluates the conjunct row by row in the RowFilter, and Spark evaluates it again above the scan, because Iceberg keeps every predicate that doesn't select whole partitions as a post-scan filter (BaseSparkScanBuilder.java). Comet's own Parquet scan leaves row-level evaluation off by default for that reason (spark.comet.parquet.rowFilterPushdown.enabled), while iceberg-rust attaches the RowFilter whenever a task has a predicate. As I read it, this PR adds that row-level cost to every query whose residual converts only in part.
Could the fixes for #7136, #7137 and the promoted-column part of #7138 go in a separate fix: PR ahead of this one? They change results that main and branch-1.1 get wrong, and the backporting guide sends such fixes to the release branch. With a perf: title, generate-changelog.py lists the PR under "Performance related" (generate-changelog.py), so the bugs @andygrove asked to track wouldn't show up as fixes. Those fixes also remove pushdown that main has today (FLOAT and DOUBLE ranges, promoted columns, NOT over equality). A benchmark of this PR as it stands would mix that loss with the gain from partial pushdown.
| These null checks provide no native row-group pruning. In a positive conjunction, supported | ||
| primitive predicates still prune row groups and pages, while the post-scan filter evaluates the | ||
| complete condition. `OR` requires a safely pushable predicate from both branches, and expressions | ||
| inside `NOT` must convert completely before they can be pushed. When only part of a condition can | ||
| be pushed, predicates on `BIGINT` and `DOUBLE` columns in it are pushed only for `=` and null | ||
| checks, because a data file can store a narrower type than its column without any record of it. | ||
|
|
||
| Tables with a `schema.name-mapping.default` property push a conjunction only when every predicate | ||
| in it can be pushed. The `add_files`, `migrate` and `snapshot` procedures set this property when | ||
| they import data files, and an imported file can store a narrower type than its column. | ||
|
|
||
| Spark treats `-0.0` as equal to `0.0` and sorts every NaN above all numbers, but iceberg-rust | ||
| uses IEEE 754 total ordering. On `FLOAT` and `DOUBLE` columns, range comparisons and comparisons | ||
| with `0.0`, `-0.0` or NaN are therefore evaluated after the native scan. | ||
|
|
||
| Some other predicates are also evaluated only after the native scan: | ||
|
|
||
| - Negated equality on a nullable column. Iceberg passes `<=>` as `=`, so for `NOT (a <=> 1)` the | ||
| native reader would drop NULL rows that Spark keeps, and `a <> 1` reaches Comet in the same | ||
| form. | ||
| - Comparisons other than `=` with a literal on a column whose type was promoted, for example | ||
| from `INT` to `BIGINT`, because older data files keep the original type. If expiring | ||
| snapshots removed older schemas, this applies to every `BIGINT` and `DOUBLE` column. A | ||
| promotion made before Iceberg 0.12, which kept no schema history, cannot be detected. |
There was a problem hiding this comment.
Could this section be shorter? It grows from two sentences to 24 lines, and most of the new text explains why each rule exists, which the code comments and the Known Issues entry already cover. It also says the kept predicates "prune row groups and pages", but iceberg-rust also uses them to filter rows during decode. Here's a version that keeps every rule a user can act on:
| These null checks provide no native row-group pruning. In a positive conjunction, supported | |
| primitive predicates still prune row groups and pages, while the post-scan filter evaluates the | |
| complete condition. `OR` requires a safely pushable predicate from both branches, and expressions | |
| inside `NOT` must convert completely before they can be pushed. When only part of a condition can | |
| be pushed, predicates on `BIGINT` and `DOUBLE` columns in it are pushed only for `=` and null | |
| checks, because a data file can store a narrower type than its column without any record of it. | |
| Tables with a `schema.name-mapping.default` property push a conjunction only when every predicate | |
| in it can be pushed. The `add_files`, `migrate` and `snapshot` procedures set this property when | |
| they import data files, and an imported file can store a narrower type than its column. | |
| Spark treats `-0.0` as equal to `0.0` and sorts every NaN above all numbers, but iceberg-rust | |
| uses IEEE 754 total ordering. On `FLOAT` and `DOUBLE` columns, range comparisons and comparisons | |
| with `0.0`, `-0.0` or NaN are therefore evaluated after the native scan. | |
| Some other predicates are also evaluated only after the native scan: | |
| - Negated equality on a nullable column. Iceberg passes `<=>` as `=`, so for `NOT (a <=> 1)` the | |
| native reader would drop NULL rows that Spark keeps, and `a <> 1` reaches Comet in the same | |
| form. | |
| - Comparisons other than `=` with a literal on a column whose type was promoted, for example | |
| from `INT` to `BIGINT`, because older data files keep the original type. If expiring | |
| snapshots removed older schemas, this applies to every `BIGINT` and `DOUBLE` column. A | |
| promotion made before Iceberg 0.12, which kept no schema history, cannot be detected. | |
| These null checks are not pushed, but the other predicates in a positive conjunction still are. | |
| iceberg-rust uses a pushed predicate to skip row groups and pages and to filter rows during | |
| decode, and the filter above the scan evaluates the complete condition. `OR` is pushed only when | |
| both branches can be, and `NOT` only when its whole child can be. | |
| Some predicates are left to the filter above the scan because iceberg-rust would return | |
| different rows than Spark: | |
| - Range comparisons on `FLOAT` and `DOUBLE` columns, and comparisons with `0.0`, `-0.0` or NaN. | |
| - `NOT` over equality on a nullable column. | |
| - Comparisons other than `=` on a column whose type was promoted. This applies to every `BIGINT` | |
| and `DOUBLE` column when only part of a condition is pushed, or when expiring snapshots removed | |
| older schemas. | |
| Tables with a `schema.name-mapping.default` property push a condition only when all of it can be | |
| pushed. |
There was a problem hiding this comment.
Applied your version, thanks.
| promotedColumns: Set[String] = Set.empty, | ||
| partialPushdown: Boolean = true): Option[OperatorOuterClass.IcebergPredicate] = { |
There was a problem hiding this comment.
Should promotedColumns and partialPushdown be required here and in serializeResidual (lines 838-839)? The one production call, at line 1263, passes both. The defaults are the least safe pair, no promoted columns and partial pushdown allowed, so a new caller that leaves them out skips the promotion guard without a compile error. As I read it, only CometIcebergNativeScanSuite relies on them.
There was a problem hiding this comment.
Both are now required in icebergExprToProto and serializeResidual, and the tests pass them explicitly.
| /** What [[icebergExprToProto]] consults at every node of a residual. */ | ||
| private case class ResidualScope( | ||
| attributes: Map[String, Attribute], | ||
| pageIndexUnsupportedColumns: Set[String], | ||
| promotedColumns: Set[String], | ||
| partialPushdown: Boolean) |
There was a problem hiding this comment.
What does ResidualScope represent? The doc comment says it's what the walk "consults at every node", which fits any argument. As I read it, it holds the settings for one of the two passes at lines 707-716, and it mixes values that are the same in both passes (attributes, pageIndexUnsupportedColumns) with values that differ (promotedColumns, partialPushdown). Meanwhile underNot, the one value that changes during the walk, stays a separate argument.
The field partialPushdown also means something different from the parameter of the same name at line 695. The parameter says whether the table allows partial pushdown at all. The field says whether this pass may drop a conjunct, and it is false on the first pass even when the table allows it. Could the type and that field get names that say what they hold (for example dropUnconvertedConjuncts for the field), with a short doc comment on each field?
There was a problem hiding this comment.
Removed it. The recursive walk now takes plain parameters instead, and its Scaladoc documents attributes, promotedColumns, dropUnconvertedConjuncts and underNot. partialPushdown is the table-level switch, and it sets dropUnconvertedConjuncts for a single pass over the residual.
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.
Push the convertible side of a positive AND when the rest of the residual cannot be pushed, such as a null check on an array column. OR still needs both branches, and NOT needs its whole child. The filter above the scan still evaluates the complete condition. Each pushed conjunct is converted exactly as it would be on its own, including the promoted-column checks. Tables with a name mapping push a residual only whole, since imported files commonly store narrower physical types that iceberg-rust compares incorrectly. Add partial filter cases, including a clustered BIGINT column, to CometIcebergReadBenchmark. Closes apache#5883.
bf663cf to
622d7e9
Compare
|
Thanks @mbutrovich. I split the fixes for #7136, #7137 and the promoted-column part of #7138 into #7156, and rebased this PR on top of it. On top of #7156, this PR only adds partial pushdown, so the benchmark below compares it with #7156, not with On "native Iceberg pruning": I also simplified the change. The serde converts the residual once, and each kept conjunct goes through the same rules as a residual that converts in full. The earlier version also restricted every Benchmark: I added partial filter cases to
The 90% scattered case was the least favorable case measured: statistics skip nothing and the Separately, while checking promoted columns I found that iceberg-rust's page-index evaluator can wrongly skip pages whose bounds have a narrower type than the table column, such as |
sunchao
left a comment
There was a problem hiding this comment.
Reviewed commit 622d7e9faecb48fe242cae087ccedf751f654a60. New P1/P2 findings are detailed inline.
| // TINYINT and SMALLINT stored as INT32 with INT_8 or INT_16 annotations, which iceberg-rust | ||
| // compares incorrectly. So partial pushdown is not extended to these tables, and they push a | ||
| // residual only when it converts whole. | ||
| val partialPushdown = metadata.nameMapping.isEmpty |
There was a problem hiding this comment.
[P1] [P1] Preserve rows from unrecorded promotions when enabling partial pushdown. A table whose id was promoted from INT to BIGINT before Iceberg 0.12 can have physical INT files, no name mapping, and only schema ID 0 in its reconstructed history. promotedColumns consequently returns empty, and this gate enables partial pushdown. For rows (1, array(1)), (2, array(2)), (3, array(3)), WHERE l IS NOT NULL AND id < 3000000000 should return all three rows. The base drops the mixed residual, but this head retains NotNull(id) AND LessThan(id, 3000000000), and the pinned reader drops every row. This extends #7138 to queries that previously returned correct results. Please retain a conservative partial-pushdown guard for potentially promoted columns unless physical types are verified, or fix the native comparison before enabling this path.
Evidence: Reproduced in /tmp/review6847-622d-1c0zesk4. LegacyPromotionProbe.scala creates an unpartitioned table with an INT Parquet file, promotes its logical column to BIGINT, and writes the supported v1 schema-only metadata representation emitted by Iceberg 0.11.1. Upstream old/current TableMetadataParser sources confirm this representation and its reconstruction as a single schema. legacy-spark.log records schema IDs [0], promoted=Set(), no name mapping, Spark results [1,2,3], base serialization None, and exact-head serialization containing the retained range. legacy-native.log records the same file returning three rows without pushdown and zero with the emitted NotNull(id) AND id < 3000000000 predicate. The reader uses iceberg-rust 1f3bc34058e3f3e45336bc5428f37b92649a608b, Arrow 59.3.0, rewrite_not().bind(...), and row selection enabled.
Which issue does this PR close?
Closes #5883.
This PR is based on #7156, which should merge first. Until then, the diff also includes that PR's changes.
Rationale for this change
A residual such as
l IS NOT NULL AND id > 5is not pushed to iceberg-rust at all today, because the null check on an array column cannot be pushed. Pushingid > 5lets iceberg-rust'sArrowReaderuse it to prune row groups using statistics, select rows using the page index and filter rows with itsRowFilterduring decode. The filter above the scan still evaluates the complete condition.What changes are included in this PR?
ANDoutsideNOT, the conjuncts that convert are kept.ORstill requires both branches to convert, andNOTrequires its entire child to convert.schema.name-mapping.default), whichadd_files,migrateandsnapshotset, keep the current behavior and push a residual only if it converts in full. Imported files often store narrower physical types, such as HiveTINYINTasINT32with anINT_8annotation, which iceberg-rust compares incorrectly.CometIcebergReadBenchmark.Partial pushdown shares the existing #7138 limitation: a promotion made before Iceberg 0.12 leaves no schema history, so comparisons on such a column can still drop rows, as they already can for a residual that converts in full.
How are these changes tested?
Serializer tests cover partial
AND,ORand nestedNOT, identical pushdown ofBIGINTandDOUBLEconjuncts with or without an unconvertible sibling, recorded promotions, and the name-mapping switch. Native tests check Spark parity for mixed residuals and that the reader prunes: on one file with several row groups, a partial residual reads less than half the bytes of a full scan, and aBIGINTrange next to an unconvertible conjunct reads the same bytes as the range alone.Local (Spark 4.1 / Scala 2.13 / JDK 21):
CometIcebergNativeScanSuiteandCometIcebergNativeSuitepassed (174 passed, 2 canceled by version assumptions).-Pspark-3.5,delta -Pstrict-warningstest-compile passed. Other Spark profiles and the Iceberg Spark SQL suites were not run locally;run-iceberg-testsis applied.Benchmark (
CometIcebergReadBenchmark --filter-only, Apple M3 Pro, 16M rows in 8 files; each file spans the whole key range, so Iceberg's file pruning skips nothing). Each query isl IS NOT NULL AND <range>, and only the range can be pushed. Each timing cell shows the best Comet time from each of two runs (run 1 / run 2):INTcolumnBIGINTcolumnINTcolumnINTcolumnOn the clustered columns, statistics and the page index skip most of the data. On the scattered column they skip nothing, and the
RowFilterevaluates the range row by row. The 90% scattered case was the least favorable case measured: the pushed range removes few rows, and the query was 8–10% slower in these runs. Within a single build, the same query with the range pushed and with it written asscattered + 0(so it is not pushed) differed by 0–5%, so part of that gap is run-to-run variation. For a range alone, which is already pushed onmain, the timings with and without pushdown overlapped (1332–1350 ms vs 1312–1387 ms).