laserninja commented on PR #12241:
URL: https://github.com/apache/gravitino/pull/12241#issuecomment-5153065714
@roryqi thank you - the determinism point was not a theoretical one, and
chasing it found a correctness bug. All three of your asks are addressed; the
document is updated and #12194 has the fix.
**The total order was not total.** You asked me to either cite why two
semantically distinct tasks cannot share `(data file location, start, length)`,
or add a tie-breaker. It cannot be cited, because they can: appending one data
file path twice leaves two manifest entries that tie on all three fields while
carrying different sequence numbers, and potentially different delete files.
Planning such a table shows it directly:
```
location=…/dup.parquet start=0 length=10 dataSeq=2 manifest=…-m0.avro pos=0
location=…/dup.parquet start=0 length=10 dataSeq=1 manifest=…-m0.avro pos=0
```
Sorting is stable, so tied tasks keep the order Iceberg planned them in -
precisely the order that is not reproducible. Two of them either side of a
batch boundary can swap between the plan and a later re-plan, and the client
then receives one twice and never sees the other. That is a wrong result, not a
slow one, so thank you for pushing on it.
Fixed in df9a2c065 by sorting on `(location, start, length, data sequence
number, file sequence number, manifest location, entry position)`. A manifest
entry is unique within a snapshot, so the order is now total for any two tasks
that are not interchangeable; sequence numbers come first because they survive
a rewrite into new manifests. The regression test builds a table holding the
same path twice and asserts which entry each batch gets - it fails on the old
comparator (`expected: <[2]> but was: <[1]>`) and passes on the new one.
**Multi-replica behaviour is now its own section (§5.15).** It separates the
two halves of your point: correctness does not depend on which replica serves a
request, and cost does. It states the worst case (`ceil(N/B)` full plans spread
across replicas, since the cache is node-local and the Iceberg client fetches
plan tasks concurrently), the production posture as a table (enable a cache,
route a scan to one replica, raise the batch size, coalesce concurrent
redemptions on a replica), and explicitly that those shrink the multiplier
without removing it. Your signing observation is recorded there too: if we ever
sign a plan task, the key must be shared by every replica, or verification
succeeds only on the issuing replica and clients see `404`s that depend on
routing.
**On removing the amplification rather than shrinking it (§8.5).** I agree
with your reply to @lasdf1234 that an internal cache cannot cure the multi-node
case. The way to cure it is to change what a plan task *is*: name a data
manifest, or a manifest plus an entry range, instead of an offset into a global
task list. Redemption then reads one manifest plus the delete manifests, so its
cost is independent of plan size, on any replica, with no shared cache - and it
makes the whole ordering question moot, because there is no global index to
stabilise. I did not propose it for this PR because Iceberg does not expose
enough to build it: `ManifestGroup` and `DeleteFileIndex`, which do residual
evaluation and delete attachment, are package-private in `iceberg-core`, so it
needs an upstream API or a careful port. It is written up as a follow-up.
**Polaris comparison (§8.4).** I looked, and there is nothing to compare
against yet: Polaris excludes `.../plan`, `.../plan/{plan-id}` and `.../tasks`
from the catalog spec it generates its API from, marked "Not implemented in
Polaris", and omits them from its advertised `/v1/config` endpoints, so clients
fall back to client-side planning. Their tracking issue, apache/polaris#966,
has been open since February 2025 with no comments, through spec refreshes up
to Iceberg 1.11. So they have neither problem you raised, for the reason that
they do not plan server side.
The implementation that does exist is Iceberg's own `CatalogHandlers` with
`RESTCatalogAdapter`, which keeps plans in a static `InMemoryPlanningState` and
resolves a plan task by map lookup. That is the exact inverse trade-off:
redemption is free, but a plan task is meaningless after a restart or on
another process. I would rather pay re-planning cost than ship that, but the
section states both sides so a reader can disagree with the choice.
--
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]
