JingsongLi commented on code in PR #9445:
URL: https://github.com/apache/paimon/pull/9445#discussion_r3886642907
##########
paimon-python/pypaimon/write/table_update.py:
##########
@@ -166,6 +166,26 @@ def _update_by_arrow_with_row_id(
self.table, self.commit_user, commit_identifier,
).update_columns(table, cols)
+ def _update_by_arrow_batches_with_row_id(
+ self, tables: Iterable[pa.Table], commit_identifier: int
+ ) -> List[CommitMessage]:
+ updater = None
+ try:
+ for table in tables:
+ cols = self.update_cols if self.update_cols is not None else [
+ c for c in table.column_names
+ if c != SpecialFields.ROW_ID.name
+ ]
+ if updater is None:
+ updater = TableUpdateByRowId(
+ self.table, self.commit_user, commit_identifier)
+ updater.update_columns(table, cols)
Review Comment:
[P1] Coalesce updates that target the same file
Calling `update_columns` independently for each input batch is only safe
when the batches map to disjoint `first_row_id` groups. If two batches update
different rows in the same base file, each call rereads the same snapshot
baseline and writes a full delta for the same `write_cols` and `first_row_id`;
after both messages are committed, one batch's changes are lost. I reproduced
this on the current head with a real batch table: batch 1 updates row 0 from
age 25 to 26, batch 2 updates row 1 from 30 to 31, and the committed table
reads `[26, 30, 35, 40, 45]` instead of `[26, 31, 35, 40, 45]`. The new
mock-only test does not exercise file generation or reads, so it misses this.
Please coalesce updates per target `first_row_id` and write one delta per
target file (or explicitly reject batches whose target groups overlap), and add
this same-file end-to-end case.
--
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]
