[python] Fix quadratic cost of writing multiple batches - #9375
Conversation
| self._write_data_to_file(self.pending_data) | ||
| self.pending_data = None | ||
| if self._buffer.num_rows > 0: | ||
| self._write_data_to_file(self._buffer.take()) |
There was a problem hiding this comment.
[P2] Preserve buffered rows until the prepare flush succeeds
take() resets the buffer before _write_data_to_file performs fallible file I/O. For a reusable StreamTableWrite, a transient storage error followed by retrying prepare_commit on the same writer silently omits these rows: with a fail-once writer, this head leaves 0 rows buffered and the retry writes nothing, while the base version retains all 3 rows and writes them on retry. Please materialize without draining, perform the write, and reset only after it succeeds (or explicitly poison/abort the writer and reject reuse).
There was a problem hiding this comment.
Good catch, fixed. Neither prepare_commit nor close drains up front any more: prepare_commit resets after the write returns, and close keeps its finally reset — both matching master. Regression test added.
Same pattern in KeyValueDataWriter._flush_all (present on master too), fixed as well but differently: _roll_write spans several files, so keeping every row would make a retry rewrite the ones that already landed. The buffer now keeps only the rows no file has taken. Safe because the sort and the fold are idempotent on their own output — PKs are unique once folded.
BlobWriter.take() is fine as-is: it only runs inside write(), which aborts on failure.
|
|
||
| def _close_current_writers(self): | ||
| has_normal = self.pending_normal_data is not None and self.pending_normal_data.num_rows > 0 | ||
| normal_data = self._normal_buffer.take() |
There was a problem hiding this comment.
[P2] Keep the normal and sidecar flush failure-atomic
_normal_buffer.take() discards the normal rows before the normal file is written. If that write fails, the vector buffer remains pending; retrying prepare_commit then has no normal_meta, flushes the vector files, and skips the row-count consistency check, so it can return sidecar-only metadata and lose the normal half. Please retain the normal table until the coordinated flush succeeds, or abort/poison the composite writer on any phase failure. The same ordering issue is present in DedicatedFormatWriter._close_current_writers at its _normal_buffer.take() call.
There was a problem hiding this comment.
Fixed the same way: _close_current_writers resets at the end, once the normal file and the sidecars have both landed. Regression test added.
|
cc @XiaoHongbo-Hope to take a review. |
| # Cleared at the end, once the normal file and the vector sidecars have | ||
| # both landed: a failure in either half has to leave the normal rows | ||
| # buffered, or a retry would commit sidecar-only metadata. | ||
| normal_data = self._normal_buffer.materialize() |
There was a problem hiding this comment.
Sidecar failure can duplicate normal data and break normal-sidecar consistency on retry. Please make the composite flush retry-safe.
| self._write_data_to_file(data) | ||
| self._buffer.reset() | ||
| return | ||
| self._write_data_to_file(data.slice(0, split_row)) |
There was a problem hiding this comment.
Changelog failure can commit the same data chunk twice on retry, making data and changelog inconsistent. Please make the data/changelog flush retry-safe.
Purpose
Linked issue: None
Filling one data file with
Mbatches costsO(M^2). Sowrite_arrow_batchin a loop gets slower with every call, even when the data is nowhere neartarget-file-sizeand no file ever rolls.Mcalls with 100 rows each, plusprepare_commitand commit, unaware table, default options:Doubling
Mused to quadruple the time; now it doubles it — quadratic became linear. Per call: 810 µs atM=500 and 3160 µs atM=2000 before, a flat ~64 µs either way after.Root Cause
Every
writemerged the incoming batch into the pending table, then measured that table to decide whether to roll the file:_merge_dataispa.concat_tablesin every implementation, andconcat_tablesdoes not combine data — the result just references the chunks of both inputs. So after theM-th write the pending table holdsMchunks per column.nbytesadds up the bytes the table's buffers reference, so reading it walks allMchunks. That makes the rolling checkO(M)on theM-th write, and it runs on every write:1 + 2 + ... + M=O(M^2). The concats themselves are cheap — 1% of an isolated 2000-write run, since they only copy chunk pointers — and thenbyteswalks are the other 99%.The check needs two numbers, total bytes and total rows, and it almost always answers "don't roll". The rows only have to be one table when a file actually rolls.
Changes
WriteBuffer(write_buffer.py): appends go into a list with runningnbytes/num_rowstotals, and concatenation happens only inmaterialize/take. The totals are exact rather than estimates —concat_tablesonly collects chunks, so the per-table sizes sum to what the concatenated table reports (a test asserts this)._check_and_roll_if_neededgates on the running totals and materializes only once a threshold is crossed. This is the condition the old loop reached by computingsplit_rowand breaking when it landed atnum_rows, so which writes roll is unchanged and_find_optimal_split_pointstill sees the same table.pending_databecomesself._bufferacross all five writers. That removes four copies of the same "if the buffer is empty assign, else concat" block, sinceWriteBuffer.appendtreats the first table like any other. A newDataWriter.pending_row_countreads the buffered row count without materializing.BlobWriterstill drains on every write: its roll decision depends on external blob bytes, which the buffered descriptors say nothing about.prepare_commitcan be retried.DataWriter.prepare_commitand both composite writers'_close_current_writersreset at the end.KeyValueDataWriter._flush_allspans several files, so it keeps the rows no file has taken yet instead — a retry resumes rather than rewriting.WriteBuffer.appendrejects a schema mismatch.TableWrite._validate_pyarrow_schemaadmits differencesconcat_tablesrejects (nullability,binaryvsfixed_size_binary), and those used to fail inwrite, which aborts and cleans up; deferring the fold would move them toprepare_commit, which has no handler, orphaning already-rolled files.Schema.equalshas the same tolerance as concat — ignores metadata, enforces nullability — so nothing the old path accepted is newly rejected.Tests
New
pypaimon/tests/write/write_buffer_test.py: 200 writes fold zero times while every rolling trigger still fires, and fail-once writers cover the failure paths — a failedprepare_commitkeeps its rows for the retry, and a PK flush that fails on its second file leaves exactly the unwritten remainder.test_write_merge_buffer.pymigrated toWriteBuffer.API and Format
No format or on-disk change.
DataWriter.pending_datais replaced by the private_bufferplus a publicpending_row_count;DataWriteris internal topypaimon.write.writer.Documentation
N/A