XiaoHongbo-Hope commented on PR #8826: URL: https://github.com/apache/paimon/pull/8826#issuecomment-5214160484
> I think this can be simplified further by keeping row-id range planning as a passive planning primitive, instead of making `RowIdRangeContext` own the read/update/skip sequence and commit lifecycle. > > The existing row-id update path already carries the information needed by the normal commit conflict detector: > > * `CommitMessage.check_from_snapshot` identifies the snapshot from which the update was planned. > * The new files in the message carry `first_row_id`, `row_count`, and `write_cols`. > * `FileStoreCommit` can therefore derive row-range × write-column conflicts and detect missing or replaced anchor files from the ordinary commit messages. > > The important requirement is that the writer must build its `CommitMessage` from the same pinned snapshot/file information used by the range planner. It must not re-plan against the latest snapshot after the user has read an older pinned snapshot. This is a writer/planner concern; it does not require `commit()` to accept additional range arguments. > > A simpler API shape would be: > > ```python > with plan_row_id_ranges(...) as plan: > for row_range in plan: > source = build_read( > snapshot_id=row_range.snapshot_id, > row_id_range=row_range, > ) > updates = user_process(source) > update_by_row_id_from_plan(row_range, updates, update_cols) > ``` > > Here, `RowIdRange` can be immutable metadata (plus an opaque internal planning token if needed for efficient file selection). The user controls read and processing, while the update writer creates the same normal `CommitMessage` that existing row-id updates already use. > > The current `protected_columns` and context-side read tracking introduce a stronger semantic contract. For example: > > ``` > read text -> compute embedding -> write embedding > ``` > > If another writer changes `text`, the existing row-id update semantics allow the `embedding` write because the actual write columns are disjoint. Tracking `text` through the context changes this into a transform/read-dependency conflict. That may be useful, but it is a new consistency contract rather than something required by row-id range commits themselves. If we need that contract, I think it should be specified and justified independently, and the dependency columns could be supplied explicitly rather than inferred through a stateful context. > > If we retain the existing row-id update conflict semantics for the first version, we should be able to remove most of the operation-specific machinery: > > * context-side `_read_columns`, `_updated`, `skip()`, and sequence state; > * the operation checkpoint and exact-snapshot return path; > * `protect_from_external_rewrites` state added to the generic commit layer. > > The range planner, pinned-snapshot tag, file-group-aligned packing, and normal `CommitMessage` conflict detection should be sufficient. This keeps the abstraction focused: planning returns ranges, reading reads a pinned range, and committing commits ordinary row-id update messages. Thanks for your very kind and detail review and explanation, which helps me a lot. Will check more carefully in my future PR too. -- 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]
