[python] Write postpone batches to fixed buckets - #8985
Conversation
4cc6177 to
77ffa6b
Compare
|
Thanks for working on this. The overall direction is aligned with the Java Spark fixed-bucket batch path (reuse existing bucket counts, include active postpone rows for append estimation, carry 1. Bucket planning is not global for chunked or distributed batch writes
I reproduced this with the same logical input and This differs from Java Spark's This also affects Daft. Ray avoids this because this PR adds a driver-side plan, but the generic batch API and Daft do not. Bucket planning needs to happen once over the complete input before writing. Daft likely needs a coordinated driver plan similar to Ray, or should retain the legacy postpone-bucket path until such planning is available. Please also add coverage for multiple 2.
|
Thanks for the review.
|
|
|
Thanks for the update. I re-tested the two original issues: multiple I found one remaining API consistency issue: With This means the bucket count depends on which public write API is used. Calling One additional resource concern: |
Thanks for the follow-up. write_row() now joins the deferred bucket planning with Arrow inputs. Added pure-row and mixed-input tests, and documented the local buffering memory bound. Existing-bucket partitions still write incrementally. cc @JingsongLi |
JingsongLi
left a comment
There was a problem hiding this comment.
You might need to introduce some abstraction—right now, a bunch of Postpone implementations are tightly coupled within the common classes TableWrite and BatchTableWrite.
Got it, updated |
JingsongLi
left a comment
There was a problem hiding this comment.
Is it possible to provide a higher-level API to accomplish this?
Sure, done |
|
You can take a review to #8992 |
5b12f13 to
b67902f
Compare
b67902f to
d8f76e7
Compare
| return StreamReadBuilder(self) | ||
|
|
||
| def new_batch_write_builder(self) -> BatchWriteBuilder: | ||
| if (self.bucket_mode() == BucketMode.POSTPONE_MODE |
There was a problem hiding this comment.
Please not do this. By default, we should not use this mode.
There was a problem hiding this comment.
Please not do this. By default, we should not use this mode.
Updated
JingsongLi
left a comment
There was a problem hiding this comment.
Thanks for making the fixed-bucket write path explicit. I revalidated the latest head (acf7e4e) against the Java implementation. The API-default concern is addressed, but the four issues below remain. The first can cause cross-engine correctness failures; the others can produce different bucket counts, stale partition metadata, or leaked files under concurrent commits.
| partitions = self._extract_partitions_batch(data) | ||
| columns = [data.column(i) for i in self.bucket_key_indices] | ||
| buckets = [ | ||
| _bucket_from_hash( |
There was a problem hiding this comment.
[P1] Respect the configured bucket function. This extractor always applies the default BinaryRow hash, while Java's PostponeFixBucketProcessor constructs BucketFunction from CoreOptions and therefore supports default, mod, and hive. For a valid bucket-function.type=mod or hive table, Python and Java can route the same primary key to different buckets; Java bucket pruning then follows the configured function and may miss rows written by Python. Please dispatch according to bucket-function.type (with Java-compatible semantics), or reject non-default functions until they are supported, and add cross-engine fixtures for all supported functions.
There was a problem hiding this comment.
[P1] Respect the configured bucket function. This extractor always applies the default BinaryRow hash, while Java's
PostponeFixBucketProcessorconstructsBucketFunctionfromCoreOptionsand therefore supportsdefault,mod, andhive. For a validbucket-function.type=modorhivetable, Python and Java can route the same primary key to different buckets; Java bucket pruning then follows the configured function and may miss rows written by Python. Please dispatch according tobucket-function.type(with Java-compatible semantics), or reject non-default functions until they are supported, and add cross-engine fixtures for all supported functions.
Fixed
| if len(indices) == data.num_rows | ||
| else pa.compute.take(data, pa.array(indices, type=pa.int64())) | ||
| ) | ||
| stats[partition] = (len(indices), partition_data.nbytes) |
There was a problem hiding this comment.
[P2] Use the same size metric as Java when deriving the bucket count. RecordBatch.nbytes measures Arrow buffer memory, whereas Java sums BinaryRow.getSizeInBytes(). These values differ materially; for example, 1,000 (INT, STRING, STRING) rows occupy 14,000 bytes by this metric but 32,000 bytes as Java BinaryRows, so a 20 KiB target plans 1 bucket in Python and 2 in Java. Because the first writer persists the partition's bucket count, engine order changes the table layout. Please use a BinaryRow-compatible serialized-size calculation (or an exactly equivalent vectorized calculation) and verify it with shared fixtures.
| partition=partition, | ||
| bucket=msg.bucket, | ||
| total_buckets=self.table.total_buckets, | ||
| total_buckets=total_buckets, |
There was a problem hiding this comment.
[P2] Preserve the replacement bucket count in partition statistics. This list emits DELETE entries first with the old total_buckets, then ADD entries with the new value. _generate_partition_statistics records only the first value it sees for a partition, so an overwrite from postpone mode (-2) to N leaves catalog partition statistics at -2 even though the new manifest entries use N. Java's PartitionEntry.merge takes the later entry's totalBuckets. Please make the statistics aggregation last-wins (and add a catalog-statistics assertion for -2 -> N, ideally also N -> M).
There was a problem hiding this comment.
[P2] Preserve the replacement bucket count in partition statistics. This list emits DELETE entries first with the old
total_buckets, then ADD entries with the new value._generate_partition_statisticsrecords only the first value it sees for a partition, so an overwrite from postpone mode (-2) toNleaves catalog partition statistics at-2even though the new manifest entries useN. Java'sPartitionEntry.mergetakes the later entry'stotalBuckets. Please make the statistics aggregation last-wins (and add a catalog-statistics assertion for-2 -> N, ideally alsoN -> M).
Fixed
| if any(message.total_buckets is not None | ||
| for message in commit_messages): | ||
| # Detect concurrent bucket-count changes in postpone APPENDs. | ||
| detect_conflicts = True |
There was a problem hiding this comment.
[P2] Keep deterministic CAS failures on the abortable conflict path. Enabling conflict detection here exposes a cleanup gap: when atomic_commit returns False, _try_commit_once creates RetryResult(exception=None) because the snapshot definitely was not committed. If the retry then detects a bucket-count conflict, _try_commit converts it to a generic exception solely because retry_result is non-null. TableCommit aborts files only for CommitConflictError, so the losing writer's files can be orphaned. Please distinguish retry_result.exception is None from an uncertain commit exception and retain CommitConflictError for the former; add a concurrent different-plan test that also asserts cleanup.
There was a problem hiding this comment.
[P2] Keep deterministic CAS failures on the abortable conflict path. Enabling conflict detection here exposes a cleanup gap: when
atomic_commitreturnsFalse,_try_commit_oncecreatesRetryResult(exception=None)because the snapshot definitely was not committed. If the retry then detects a bucket-count conflict,_try_commitconverts it to a generic exception solely becauseretry_resultis non-null.TableCommitaborts files only forCommitConflictError, so the losing writer's files can be orphaned. Please distinguishretry_result.exception is Nonefrom an uncertain commit exception and retainCommitConflictErrorfor the former; add a concurrent different-plan test that also asserts cleanup.
Fixed
JingsongLi
left a comment
There was a problem hiding this comment.
I re-reviewed the latest head (a80974c) against the Java implementation and revalidated the previous findings. The non-default bucket-function case now fails fast, the overwrite partition statistics fix matches Java, and the single deterministic CAS-failure cleanup case is covered. Two P1 issues remain: one can delete files from a commit whose result was previously uncertain, and the other makes size-based planning reject valid schemas supported by Java.
| f"after {elapsed_ms} millis with {retry_count} retries, " | ||
| f"there maybe exist commit conflicts between multiple jobs." | ||
| ) | ||
| if retry_result is not None and retry_result.exception is None: |
There was a problem hiding this comment.
[P1] Preserve commit uncertainty across all retries before aborting files. _try_commit replaces retry_result after every attempt, so an earlier commit exception (which may have occurred after the snapshot was successfully committed) can be overwritten by a later CAS False result with exception=None. If duplicate detection cannot read the original snapshot because it has expired or is temporarily unavailable, this branch raises CommitConflictError, and TableCommit aborts the data files even though a later snapshot can still reference them through its base manifests. I reproduced: attempt 1 commits snapshot 1 then loses the response; another writer creates snapshot 2; snapshot 1 is unavailable to _is_duplicate_commit; attempt 2 returns False; the final CommitConflictError deletes snapshot 1's data file while snapshot 2 remains latest. Please keep a monotonic commit_result_may_be_uncertain state across the whole retry loop (cleared only after duplicate-success confirmation) and add this multi-attempt regression test.
| data_size = 0 | ||
| if collect_size: | ||
| values = [column[row].as_py() for column in columns] | ||
| data_size = len(GenericRowSerializer.to_bytes( |
There was a problem hiding this comment.
[P1] Do not use the atomic-only GenericRowSerializer for full-row size planning. This now serializes every input field, but that serializer rejects non-AtomicType values and timezone-aware timestamps; Java uses InternalSerializers.create(rowType), which supports ARRAY, MAP, ROW, VECTOR, VARIANT, BLOB, and TIMESTAMP_LTZ. I reproduced an ARRAY<INT> value failing with ValueError: BinaryRow only support AtomicType, and an id primary-key table with a non-key TIMESTAMP_LTZ value succeeds through the legacy postpone writer but fails here with datetime tzinfo not supported yet. Ray enters this size-based planner by default, so valid existing postpone writes regress before routing any rows. Please use a Java-compatible full type-surface size calculator/serializer and add nested-value and non-key TIMESTAMP_LTZ fixtures in addition to the current all-atomic 32,000-byte fixture.
Purpose
Postpone-bucket files are not visible to normal readers until compaction. Java batch integrations solve this through a dedicated postpone fixed-bucket write builder.
This adds the same explicit builder to PyPaimon and uses it from the Ray batch integration. The regular
new_batch_write_builder()keeps its existingbucket-postponebehavior.Changes
new_postpone_fixed_bucket_write_builder()forbucket=-2tables.postpone.batch-write-fixed-bucket.max-parallelism.(partition, bucket), and pass the plan to explicit worker writers.Tests
table_write_test.pyray_sink_test.pyray_repartition_test.pydaft_pk_distributed_write_test.py