eneskeles commented on PR #2111: URL: https://github.com/apache/iceberg-go/pull/2111#issuecomment-6023926079
> Where the filtered column is present, the three-valued complement is correct and matches Spark's copy-on-write semantics (`Not(EqualNullSafe(cond, true))`), but two problems remain: when the filter contains `IsNaN`, every row of files written before that column was added is deleted, and whole-file deletes still drop NULL rows for `!=`/`NOT IN`. > > **Prior feedback** > > * `@CaptainAni187`'s NaN row-group pruning case is fixed in [31dff2f](https://github.com/apache/iceberg-go/commit/31dff2f232ac268d723988914c59c4aa08d7b515): the complement is now `x >= 5 OR is_nan(x) OR x IS NULL`, and `TestCopyOnWriteDeleteKeepsNaNRowsInPrunedRowGroups` covers it. > > ### Whole-file deletes still drop NULL rows for `NotEqual` / `NotIn` > The rewrite keeps rows where the filter is NULL now, but a file only gets rewritten if the strict metrics evaluator says some row might not match. For `NotEqual`/`NotIn`, `strictMetricsEval.VisitNotEqual`/`VisitNotIn` (`table/evaluators.go`) return `rowsMustMatch` without checking for nulls, either when the column is all-NULL or when the file's bounds exclude the literal(s). Those are Iceberg's two-valued semantics, which is why the Java Spark integration ANDs `notNull(col)` onto SQL `NOT IN` ([`SparkV2Filters`](https://github.com/apache/iceberg/blob/main/spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkV2Filters.java)). This PR adopts SQL semantics instead. `RewriteNotExpr` sends `NOT(EqualTo)` and `NOT(In)` through the same path. > > As a result, whether a NULL row survives depends on the other values in its file. With `age = [25, NULL, 40]` in one file, on both v2 and v3: > > * `Delete(NotEqualTo(age, 30))`: the file is rewritten and the NULL row is kept. > * `Delete(NotEqualTo(age, 100))` or `Delete(NewNot(EqualTo(age, 100)))`: the whole file is deleted, NULL row included, even though a scan with the same filter only returns ids 1 and 3. > > Since this PR closes #2110, I'd fix it here. Building the strict check from the complement puts both paths on the same semantics: > > ```go > notTrue, err := isNotTrueExpr(filter) > if err != nil { > return nil, nil, nil, err > } > strictEvaluator, err := newStrictMetricsEvaluator(schema, iceberg.NewNot(notTrue), caseSensitive, false) > ``` > > `NOT(age = 100 OR age IS NULL)` rewrites to `age != 100 AND age IS NOT NULL`, so a file containing nulls goes to the rewrite instead. I tried this locally: the cases above keep the NULL row, and the existing `Delete`/`Overwrite`/copy-on-write tests in `./table` still pass. Because the code is shared, this also changes merge-on-read classification. I think that's correct too: MoR's position-delete pass already keeps the NULL row for `age != 30`, and only the whole-file path drops it. If you'd rather not take this on here, please file a follow-up and use `Refs #2110` instead of `Fixes`. Either way, please add a test where the literal falls outside the file's bounds. > > **Smaller observations** > > * The `Delete` doc comment says partially matching files keep "only non-matching rows". Under copy-on-write, rows where the filter evaluates to NULL now count as non-matching. That's the user-visible change, so the comment should say it. > > Details inline. > > > _This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in._ > > _More on how Apache Iceberg handles maintainer review:_ [CONTRIBUTING.md](https://github.com/apache/iceberg-go/blob/main/CONTRIBUTING.md). Thanks for the review, both problems reproduce on my side. Pushed e4b0394. - **Whole-file deletes:** took your suggestion, the strict check is now built from the complement, so `Fixes #2110` stays. With `age = [25, 30, NULL, 40]`, `Delete(age != 100)`, `Delete(NOT(age = 100))` and `Delete(NotIn(age, 100, 200))` now leave only the NULL row. Added those three as test cases, with the literal outside the file's bounds. - **Merge-on-read:** I checked the shared classification there too. `Delete(x != 100)` on a file with a NULL used to drop the whole file, and now writes position deletes and keeps the NULL row. - **`IsNaN` on files without the column:** applied your suggestion and added a schema-evolution test. - **Doc comment:** `Delete` now says that rows where the filter evaluates to NULL do not match and are kept. - **`NotIn`:** filed #2132 for the multi-valued case and referenced it in the code comment. One side effect: in copy-on-write, `Delete(x != 100)` on a file where `x` is all NULL now rewrites the file with the same rows instead of deleting it. That is correct but does a rewrite that changes nothing. I left it as is, since avoiding it would mean changing the inclusive check too. While testing the schema-evolution case I also found that a plain scan with `NotNaN(x)` skips the rows of files written before `x` was added, though it returns rows with a stored NULL. That is not from this PR, so I filed #2133. -- 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]
