XiaoHongbo-Hope commented on PR #659: URL: https://github.com/apache/paimon-rust/pull/659#issuecomment-5238670843
> Thanks for addressing the overlapping-writer correctness issue in `afbb593`. I rechecked the new Rust and C paths: the commit-time guard is applied to append, expected-snapshot commit, and overwrite, and the focused Rust/C regressions pass. > > 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. > > 1. **`PostponeFixedBucketWriter` is doing much more than writing.** It loads table metadata, derives bucket counts, buffers batches, routes rows, and manages the one-shot lifecycle through `plan_provided`, `metadata_loaded`, and `prepare_started` ([code](https://github.com/apache/paimon-rust/blob/afbb593788dab120cce864b1f710a1b677344a14/crates/paimon/src/table/postpone_batch_table_write.rs#L235-L429)). PyPaimon separates these responsibilities into `PostponeBucketPlanner`, `PostponeFixedBucketRowKeyExtractor`, and `PostponeFixedBucketBatchTableWrite`. Java moves planning/routing even further toward the execution layer through `PostponeFixedBucketChannelComputer` and `PostponeBatchWriteOperator`. > 2. **The generic `TableWrite` is now coupled to this special mode.** It contains an optional fixed-bucket sidecar, asks that sidecar to plan during `prepare_commit`, feeds the routed batches back through the generic `write_bucket` path, and later queries the sidecar again to populate `CommitMessage.total_buckets` ([code](https://github.com/apache/paimon-rust/blob/afbb593788dab120cce864b1f710a1b677344a14/crates/paimon/src/table/table_write.rs#L841-L908)). This makes the control flow and lifecycle harder to follow than a dedicated `PostponeFixedBucketTableWrite` which composes the normal file writers. > 3. **The stated low-level scope does not quite match the implementation boundary.** The PR now says that global planning and shuffling belong to integrations, but the core writer still performs local metadata scans, buffering, planning, and routing. If this is truly a low-level primitive, I would expect it to consume a resolved plan (or preassigned bucket plus `total_buckets`) and leave planning/topology outside. If local single-process convenience is also required, it could be a separate wrapper around that lower-level primitive. > 4. **Ownership is detected after writing rather than established by topology.** The new `TableCommit` validation is useful as a defensive backstop, but by that point conflicting workers have already produced files. The same invariant is also duplicated in the C message merge API ([code](https://github.com/apache/paimon-rust/blob/afbb593788dab120cce864b1f710a1b677344a14/bindings/c/src/write.rs#L588-L634)). Java establishes ownership in the channel computer, while PyPaimon establishes it through Ray preclustering before worker writers run. The commit check should remain, but it should not be the primary ownership mechanism. > 5. **The builder does not describe one coherent operation.** `with_overwrite()` changes writer-side planning, but `new_commit()` still returns an unconfigured `TableCommit`; the caller can accidentally pair an overwrite-planned writer with `commit()` instead of `overwrite()`. Both Java and PyPaimon carry the overwrite/static-partition mode into the writer and committer created by the builder. > > 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: > > * a standalone planner producing an immutable resolved plan; > * a pure router mapping `(partition, bucket key, plan)` to a bucket; > * a dedicated fixed-bucket table writer consuming that plan/router; > * integration-owned selection and `(partition, bucket)` shuffling; > * commit ownership validation retained only as a final safety check. > > 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 `TableWrite`, `TableCommit`, `CommitMessage`, and the C merge layer, and makes it look structurally different from both reference implementations. Got it -- 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]
