Neuw84 commented on PR #18622:
URL: https://github.com/apache/iceberg/pull/18622#issuecomment-6368032162
@JeonDaehong you're right, and thanks for the careful reproduction — the two
orders problem and the `WHERE u = <uuid>` regression are real, and I should not
have changed `Comparators` in isolation.
To restate your finding so it's on the record:
- My change flips only `Comparators.forType(UUID)` to unsigned. That's the
write/bounds side: `PartitionSummary` builds manifest-list partition bounds
through `Comparators.forType`, so they now come out in unsigned order.
- But the read side — `ManifestEvaluator`, `InclusiveMetricsEvaluator`,
`ParquetMetricsRowGroupFilter` — compares through `lit.comparator()`, and
`Literals.UUIDLiteral` is still a `ComparableLiteral` using signed
`UUID.compareTo`. (Note `FixedLiteral`/`BinaryLiteral` already use
`Comparators.unsignedBytes()`; UUID is the one 16-byte type left on signed
order.)
- So the two sides disagree, and your `identity(u)` scan shows the
consequence: an unpartitioned file with bounds `[00…0001, ff…fffe]` and
`equal("u", c0…)` now plans 0 files on this PR where it planned 1 on main. That
is a regression I introduced, in exactly the plain `WHERE u = …` case from
#14216. I confirmed the mechanism matches what you described.
So flipping `Comparators` alone is not a safe partial fix — it has to be the
literals + evaluators aligned together, which is #14500.
And that's the heart of it: the correct fix here **is** #14500. That PR
already lines up the literals, the evaluators and the write path, and —
crucially — it carries the backward-compatibility logic that a bare flip to
unsigned cannot: metadata written in signed order by Iceberg Java ≤1.11 must
still be read, which is why bodduv's approach evaluates the predicate twice
(RFC-compliant unsigned **and** signed, filtering in if either matches) and
returns `ROWS_MIGHT_NOT_MATCH` for UUID in `StrictMetricsEvaluator`. My
one-line comparator change has none of that, so even "finish it properly in
this PR" would mean re-implementing #14500 — and #14500 was explicitly held
pending a dev-list discussion and a community-sync decision on how UUID
ordering should be defined in the spec, since it is a cross-version behavioral
change.
I don't think this PR should race ahead of that. Two ways forward, and I'd
like your (and ideally @bodduv's) read:
1. **Fold the equality-delete fix into #14500.** The `DeleteFileIndex` drop
you confirmed as correct here is the same root cause as the `WHERE u = …` drop
#14500 targets — one unsigned-ordering fix across literals + evaluators +
delete planning covers both, with the signed-metadata read compatibility
handled once. bodduv offered on Oct 8 to split out the read-path changes first;
the equality-delete case is a natural part of that read-path slice, and I'm
happy to contribute a test like
`testEqualityDeleteWithUuidKeyAttachesToDataFile` there.
2. **Keep #18622 only if it can be scoped to not touch
`Comparators.forType`** — but I don't think that scoping exists, because
`DeleteFileIndex.canContainEqDeletesForFile` → `rangesOverlap` goes through
that very comparator. Any fix to the delete drop changes UUID bounds comparison
and reintroduces the same two-orders split you found. So I lean strongly toward
(1).
My preference is to close this in favor of #14500 (or to retarget it as the
read-path slice bodduv described), rather than ship a partial flip that trades
one correctness bug for another. I'll hold the PR as-is pending that decision.
@bodduv — if you're splitting out the read path, would an equality-delete
regression test be welcome in that slice?
--
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]
