feat(write): add fixed-bucket write primitives for postpone tables - #659
feat(write): add fixed-bucket write primitives for postpone tables#659XiaoHongbo-Hope wants to merge 29 commits into
Conversation
|
|
|
Thanks @JingsongLi, fixed in a18eede. |
|
| /// Planning state for batch writes to postpone-bucket tables. Partitions with | ||
| /// an existing real-bucket count are written incrementally, while only new | ||
| /// partitions are buffered until their bucket count can be inferred. | ||
| struct PostponeFixedBucketState { |
There was a problem hiding this comment.
Create a new rs file for postpone writer.
JingsongLi
left a comment
There was a problem hiding this comment.
I found three behavior gaps compared with the merged PyPaimon implementation: distributed writers do not share a bucket plan, overwrite rescaling is rejected by conflict detection, and the size target is validated even when the row-count target should take precedence. Details are inline.
| .copied() | ||
| .unwrap_or(0) | ||
| }; | ||
| let total_buckets = infer_bucket_count( |
There was a problem hiding this comment.
Each TableWrite derives total_buckets from only its local buffered rows. However, the C API supports merging messages from multiple fixed-bucket writers that share a commit_user, and there is no way to provide those writers with one precomputed bucket plan. Two workers writing the same new partition can therefore infer different counts and make the whole commit fail; even when they infer the same count, the plan is based on shard-local rather than global batch statistics and can under-bucket the partition. PyPaimon avoids this by aggregating partition statistics on the driver and injecting the same PostponeBucketPlan into every worker. Please expose and validate a precomputed partition -> total_buckets plan in the Rust builder/writer and C API, or explicitly reject this multi-writer mode.
| check_from_snapshot: Option<i64>, | ||
| ) -> Result<()> { | ||
| self.check_delete_entries_against_base(base_entries, delta_entries)?; | ||
| self.check_total_bucket_conflicts(base_entries, delta_entries)?; |
There was a problem hiding this comment.
This bucket-count check runs unconditionally before considering commit_kind, and it compares raw base and delta entries. An overwrite contains DELETE entries for the old layout followed by ADD entries for the replacement layout, so a valid rescale such as 1 -> 3 buckets is rejected as a conflict. The upstream Java implementation skips the old-layout consistency check for OVERWRITE, and PyPaimon has test_postpone_overwrite_allows_bucket_rescale. Please continue checking that all new ADD entries in the delta agree, but compare them with the base layout only for non-overwrite commits (or apply ADD/DELETE changes before checking the final active entries).
| bucket_function_type, | ||
| max_parallelism: options.postpone_batch_write_fixed_bucket_max_parallelism()?, | ||
| target_rows_per_bucket: options.postpone_target_row_num_per_bucket()?, | ||
| target_size_per_bucket: options.postpone_target_size_per_bucket()?, |
There was a problem hiding this comment.
postpone.target-size-per-bucket is parsed and validated even when postpone.target-row-num-per-bucket is configured. The option contract says that the size target is ignored in that case, and the PyPaimon planner only reads it in the row target is None branch. With a valid row target plus an invalid or zero size target, Rust currently rejects writer creation even though the size value is unused. Please parse and validate the size target only when no row-count target is present.
JingsongLi
left a comment
There was a problem hiding this comment.
One new regression remains after the latest fixes: the core builder now auto-enables fixed mode without the global plan that PyPaimon obtains at the integration layer. Details inline. The previous three findings are fixed.
| pub fn new(table: &'a Table) -> Self { | ||
| let schema = table.schema(); | ||
| let options = CoreOptions::new(schema.options()); | ||
| let postpone_fixed_bucket = options.bucket() == POSTPONE_BUCKET |
There was a problem hiding this comment.
Why enabling fixed_bucket write by default? Can you create a real PostponeFixedBucketWriteBuilder?
|
Thanks for adding the fixed-bucket writer. The low-level writer is mostly consistent with PyPaimon, but the high-level distributed workflow is not aligned yet. [P1] The integration does not select the fixed-bucket pathPyPaimon's Ray sink automatically selects fixed-bucket writing when This PR only exposes an explicit Rust/C builder. The DataFusion integration is unchanged, and Rust does not define the corresponding boolean option. Therefore, the normal high-level Rust write path still uses postpone buckets rather than Python's fixed-bucket workflow. Could we either wire this into the high-level batch integration or explicitly scope the PR to low-level primitives rather than PyPaimon parity? [P1] Sharing the bucket-count plan is insufficient for safe distributed writesPyPaimon preclusters data by partition, bucket, and primary key before dispatching it to writers (source). This is important because independent writers targeting the same bucket can restore the same sequence-number state; PyPaimon explicitly documents direct distributed primary-key writes as unsafe without this routing (source). The new C API shares only Please add an ownership/routing contract—ideally one writer per [P2] Rust exposes a resolved plan, but not the Python-equivalent plannerPyPaimon exposes Rust exposes only One clarification: buffering in the low-level writer without a supplied plan is aligned with PyPaimon's direct writer. The missing parity is primarily the coordinator/integration layer that computes one global plan and safely partitions work across writers. Until these gaps are addressed, I would describe this PR as providing the core Rust/C fixed-bucket primitives, rather than full alignment with PyPaimon's distributed batch-write workflow. |
|
Thanks for the review. Fixed the distributed ownership issue in afbb593:
I also clarified the PR scope: this PR provides the explicit low-level Rust/C primitives. High-level integration selection, global planning, and preclustering remain integration-layer work. |
|
Thanks for addressing the overlapping-writer correctness issue in My remaining concern is architectural rather than another isolated correctness bug. I think the reason this implementation still feels quite different from both Java and PyPaimon is that it sits halfway between a low-level writer primitive and a complete batch-write pipeline.
I do not think Rust needs to copy either implementation's class hierarchy, but the responsibility boundaries should be similar. My preferred direction would be:
At minimum, I think we should decide before merging whether this PR is a genuinely low-level primitive or a local end-to-end implementation. The current middle ground is what makes the code spread across |
Got it |
|
@JingsongLi Thanks for your patient and detailed reviews throughout these iterations. The original intent was simply to allow Go users to write to However, the user has now migrated the table to fixed-bucket mode, so I have lowered this PR to medium priority. I have done self-review tonight, removed the local bucket-planning wrapper, and refactored the implementation to make the change more clear. This PR now focuses only on the low-level APIs. In a follow-up PR, I will implement bucket planning on the Go side and connect it to these APIs to complete the end-to-end Go write path. Given above information, could you help review the PR again when you are free. |
Purpose
Add explicit low-level Rust/C primitives for writing fixed buckets in postpone-bucket primary-key tables.
Changes
(partition, bucket)ownership at commit timetotal_bucketsScope
This PR does not change normal
WriteBuilder, DataFusion, or Fusion writes; they continue to usebucket = -2.Callers must provide one shared plan and route each
(partition, bucket)to a single writer. Commit-time ownership validation is the final backstop.Bucket planning, global statistics, preclustering, and distributed shuffle belong to the calling integration and are outside this PR. Java Spark staged-file planning and static or partial-partition overwrite are also out of scope.
Deletion-vector tables are not supported by this fixed-bucket path and are rejected before writing.
Verification
cargo fmt --all -- --checkcargo clippy --locked -p paimon-c --all-targets -- -D warningscargo test --locked -p paimon postpone --lib(13 passed)cargo test --locked -p paimon-c(58 passed)API and Format
No storage-format change. Fixed-bucket behavior is available only through the explicit Rust/C APIs and requires a caller-provided bucket plan.