laserninja commented on PR #12241:
URL: https://github.com/apache/gravitino/pull/12241#issuecomment-5195094735
@lasdf1234 I like this, and I think it is the right end state - it is the
direction §8.5 sketches, and your framing is sharper than mine: the `/plan`
call has already computed the whole split, so redeeming a batch should never
recompute it. It also removes the ordering problem entirely, because manifests
are immutable and a position inside one is fixed, so §5.6 stops being
load-bearing.
Before we commit to the payload, four things I ran into when checking it
against the Iceberg 1.11 APIs. Three of them come from the same decision - "no
embedded scan/snapshot" - and I think that decision has to be reversed.
**1. Without the snapshot, delete files cannot be attached.** Delete
manifests are reachable only through a snapshot
(`Snapshot.deleteManifests(io)`); a manifest path on its own does not say which
snapshot referenced it. The same applies to sequence numbers: entries inherit
them from the `ManifestFile` in the snapshot's manifest list, and
`ManifestFiles.read` needs that `ManifestFile`, not a path. So a merge-on-read
table would return tasks with no deletes attached, and the client would read
rows that have been deleted. Silently wrong results are the failure mode I
would most want to avoid here.
**2. Without the scan, the ranges do not reproduce and residuals cannot be
computed.** `ManifestReader.iterator()` yields entries *after*
`filterRows`/`filterPartitions`/`caseSensitive` are applied, so "entry 40"
means one thing with the client's filter and another without it. And
`residual-filter`, which the engine applies on top of each data file, is
derived from the scan filter (`ResidualEvaluator.of(spec, filter,
caseSensitive)`); with no filter we would have to emit always-true residuals,
which again changes query results rather than just performance.
**3. Without the table, `/tasks` would read whatever path it is handed.**
Authorization is evaluated from the table in the request path, but the manifest
paths come from the request body. A caller authorized on table A could send a
plan task naming a manifest of table B and get back B's data file paths and
column statistics. We would need to validate every manifest against the pinned
snapshot's manifest list - which needs the snapshot again.
**4. Payload size moves from bounded to unbounded.** Your example already
spans three manifests in one batch. Tables written by frequent small commits
have many small manifests, so a 100-task batch can name dozens, at roughly 100
bytes of S3 path each. The plan tasks then grow to kilobytes each, and `/plan`
carries all of them, which erodes the bounded-response property batching exists
for. Today a plan task is a few hundred bytes whatever the plan size. Capping
the manifests per plan task fixes it, at the price of uneven batches.
So the payload I would suggest is yours plus the three fields it dropped:
```json
{
"table": "db.t",
"snapshot-id": 42,
"scan": { "filter": …, "case-sensitive": true, "select": [],
"stats-fields": [] },
"ranges": [
{ "manifest": "s3://wh/db/t/metadata/snap-42-m1.avro", "entry-start":
40, "entry-end": 60 }
]
}
```
That keeps validation, residuals and delete attachment correct, and still
gets the property we are after: redemption cost is proportional to the batch,
not to the plan, on any replica, with no shared cache.
**The piece that needs care is delete attachment.** `ManifestFiles.read`,
`ManifestReader.filterRows/select/caseSensitive` and `ResidualEvaluator` are
all public, so the data side is buildable. `DeleteFileIndex` is not - it is
package-private in `iceberg-core` - so matching deletes to data files by
partition and sequence number, including equality deletes and v3 deletion
vectors, would have to be reimplemented. That is the part where a subtle
mistake returns deleted rows.
A way to get most of the win without that risk: use the manifest-scoped path
only when the pinned snapshot has no delete manifests, and fall back to the
current re-plan when it does. Copy-on-write tables - the common case, and the
ones with the largest plans - get redemption proportional to the batch
immediately, and merge-on-read keeps today's correctness until either Iceberg
exposes delete indexing or we port it deliberately with tests.
On sequencing, one point that I think takes the pressure off: a plan task is
opaque and nothing persists it, so its encoding can change in any later release
without breaking a client or needing a migration. Changing it is not an API
change. So this does not have to block #12194 - and @nevzheng made the same
point about not blocking it on the cache discussion in #12254.
I will write this up in §8.5 either way, with your payload as the starting
point, so the follow-up has a design to implement rather than a paragraph. Two
questions for you and @roryqi:
1. Do you want the manifest-scoped payload in #12194 now, or as the
follow-up it is written up as? I lean follow-up, because of the delete-index
work, but I am happy either way and will do it now if you would rather not
merge the offset form at all.
2. For the follow-up, is the copy-on-write-only first cut acceptable, or
would you rather wait and do both paths at once?
Two small corrections for whoever implements it: the plan response field is
`status`, not `plan-status`, and a redeemed plan task should omit `plan-tasks`
rather than send `[]`.
--
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]
