laserninja commented on PR #12194: URL: https://github.com/apache/gravitino/pull/12194#issuecomment-5289802140
@lasdf1234 both fair. Taking them in turn. **Splitting.** Already in progress - see [the plan above](https://github.com/apache/gravitino/pull/12194#issuecomment-5246735447). Two of the four are open: #12411 (189 lines) and #12412 (387 lines). Measured against a tree with both merged, what is left here is 1156 insertions across 14 files, of which 424 are production code and 732 are tests, plus 200 lines of federation that becomes its own PR. If 1156 is still too much, the natural next cut is three: | PR | Contents | Size | | -- | -------- | ---- | | a | `PlanTaskCodec` and its round-trip tests. Encoding only, nothing calls it yet | 292 | | b | Batching in `planTableScan`, redemption in `fetchScanTasks`, `scan-plan-task-batch-size`, batching and E2E tests | 828 | | c | Advertise `POST .../tasks` in `/v1/config`, user docs | 36 | Splitting (c) out is worth it on its own: it is the commit where the feature becomes visible to clients, so it is the one to revert if anything goes wrong, and it is small enough to review in a minute. Say the word and I will cut it this way. **`plan-task` structure.** Yes, and that is your manifest entry-range proposal from [#12241](https://github.com/apache/gravitino/pull/12241#issuecomment-5178530018) - I worked it through in §8.5 of the design doc. It is the better end state: naming manifest entry ranges lets redemption read those manifest slices instead of re-planning the snapshot, so cost scales with the batch rather than the plan, and it retires the total-ordering requirement entirely, since a position inside an immutable manifest is already stable. What keeps it out of this PR is delete attachment. `DeleteFileIndex`, which matches delete files to data files by partition and sequence number, is package-private in `iceberg-core`, so entry ranges would mean reimplementing those rules - equality deletes, positional deletes, deletion vectors - where a subtle mistake silently returns deleted rows. The staged path in §8.5.4 is to use entry ranges when the pinned snapshot has no delete manifests and fall back to re-planning when it does, which covers copy-on-write tables. The reason offset/limit now does not lock us in: a `plan-task` is opaque to clients and is never persisted server side, so the payload can change in any later release with no migration and no compatibility window. The only thing that has to be right now is that the string stays opaque, which it is. -- 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]
