JingsongLi commented on code in PR #8236:
URL: https://github.com/apache/paimon/pull/8236#discussion_r3450767370
##########
paimon-core/src/main/java/org/apache/paimon/operation/FileStoreCommitImpl.java:
##########
@@ -1190,7 +1198,8 @@ public boolean replaceManifestList(
latest.statistics(),
// if empty properties, just set to null
latest.properties(),
- nextRowId);
+ nextRowId,
+ latest.operation());
Review Comment:
`replaceManifestList` is used by maintenance/repair flows such as row-id
reassignment and removing broken manifests, but this copies the previous
snapshot's logical operation into the new maintenance snapshot. That makes the
`$snapshots.operation` value look like the previous user operation even though
this snapshot was produced by a manifest rewrite. I think this should stay
`null` (or use a dedicated maintenance operation) instead of inheriting
`latest.operation()`; the same concern applies to the manifest compaction
snapshot below.
##########
paimon-api/src/main/java/org/apache/paimon/Snapshot.java:
##########
@@ -201,7 +207,8 @@ public Snapshot(
@Nullable Long watermark,
@Nullable String statistics,
@Nullable Map<String, String> properties,
- @Nullable Long nextRowId) {
+ @Nullable Long nextRowId,
+ @Nullable Operation operation) {
Review Comment:
This removes the existing public `Snapshot(...)` constructor signature and
forces all callers to pass the new operation argument. Since `Snapshot` is
annotated `@Public`, this is a source compatibility break for external code
constructing snapshots directly. Could we keep the old constructor overload and
delegate to the new one with `null` for `operation`, while adding this extended
constructor separately?
##########
docs/docs/pypaimon/system-tables.md:
##########
@@ -91,6 +91,7 @@ One row per persisted snapshot.
| `changelog_record_count` | BIGINT |
|
| `watermark` | BIGINT |
|
| `next_row_id` | BIGINT |
|
+| `operation` | STRING | `WRITE`, `DELETE`, `MERGE`,
...|
Review Comment:
This documents an `operation` column for PyPaimon, but the Python
implementation still exposes `$snapshots` only through `next_row_id`
(`paimon-python/pypaimon/table/system/snapshots_table.py`) and the Python
`Snapshot` model does not parse the new `operation` field. As written, PyPaimon
users will not see the documented column. Could you either update the PyPaimon
snapshot model/system table/tests as part of this PR, or leave this PyPaimon
doc unchanged?
--
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]
