JingsongLi commented on PR #10297: URL: https://github.com/apache/paimon/pull/10297#issuecomment-5935496692
Production review at `1944e95879`. The end-to-end Flink feature is useful, but two issues need to be addressed before merging. **[P1] Keep compaction and recovery out of the ambiguous-routing guard** `AbstractFileStoreWrite.java:504` now calls `requirePartitionBucketCount()` before looking up an existing writer. `compact(partition, bucket, ...)` and `notifyNewFiles(...)` also use this overload, so they throw on every partitioned table with the option enabled, even after a valid explicit-count write has already created the writer. This is reachable through supported Flink paths: `GlobalFullCompactionSinkWrite.submitFullCompaction()` calls `write.compact()` for written buckets, and `LookupSinkWrite` calls it when restoring active buckets. I reproduced the actual full-compaction writer with a real local file table: a mapped write succeeds, then `prepareCommit(false, 1)` fails with the new `UnsupportedOperationException`; the same control with the option disabled succeeds. No rescale is required. Thus a full-compaction changelog job fails when compaction triggers, and a lookup job can fail during restart. Restrict the ambiguous-count rejection to routing writes. Maintenance/recovery operations must reuse a valid existing writer or resolve the actual partition count when creating one. Moving the guard after the cache lookup alone would still leave dedicated compaction and recovery of a missing writer broken. Please cover full-compaction commit, lookup restoration and `notifyNewFiles` with the option enabled. **[P2] Preserve the promised no-op for unpartitioned tables in Spark** `PaimonSparkTableBase.scala:175–178` and `PaimonSparkWriter.scala:130–133` check the option without checking whether the table is partitioned. The PR promises that the option remains a no-op for unpartitioned tables, and the core mapping/writer checks already enforce that distinction. However, a normal unpartitioned fixed-bucket table with this property set cannot accept a Spark INSERT. I exercised actual SQL with `spark.paimon.write.use-v2-write=false` and `true`: both unpartitioned INSERT/readback controls pass with the property disabled, and both fail in the new guard with it enabled. Limit the rejection to partitioned tables that actually use this feature, and align the documentation with the advertised no-op. Verification: 210 core tests, 14 Flink tests and all 10 existing Spark PaimonSinkTest cases passed with normal Maven checks, including the rescale/savepoint/restart scenarios. The failures above were additional targeted reproductions, outside the PR's existing coverage. -- 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]
