sundapeng commented on PR #10303: URL: https://github.com/apache/paimon/pull/10303#issuecomment-6008122984
Thanks for the update. I checked 2dbde5b2d, and the previous comments are all resolved. One issue is left before merge. `fileFormat` is only set by the package-private `withFileFormat` after construction ([L105-L112](https://github.com/apache/paimon/blob/2dbde5b2d779a38cf474f3a61df7ba4c38f70d20/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java#L105-L112)), so the public constructor ([L114-L142](https://github.com/apache/paimon/blob/2dbde5b2d779a38cf474f3a61df7ba4c38f70d20/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java#L114-L142)) leaves it null. This constructor is already public on master, and [FormatTableCommitCompatibilityTest](https://github.com/apache/paimon/blob/2dbde5b2d779a38cf474f3a61df7ba4c38f70d20/paimon-core/src/test/java/org/apache/paimon/table/FormatTableCommitCompatibilityTest.java#L37) keeps it usable outside the package. Callers using it should not break. With a null format: - Append is rejected. Since 25e3456c5, every builder overwrite registers `file.format`, also on single-format tables. After that, an append through the public constructor fails at [L437](https://github.com/apache/paimon/blob/2dbde5b2d779a38cf474f3a61df7ba4c38f70d20/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java#L435-L447): ``` Cannot append files in format null to partition {pt=p} of Format Table ... registered with file.format=parquet. ``` I reproduced it on a parquet-only table. Master has no such check, so this is a regression. - Overwrite does not update `file.format` ([L638](https://github.com/apache/paimon/blob/2dbde5b2d779a38cf474f3a61df7ba4c38f70d20/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java#L638-L642)). If the partition was registered with another format, the commit succeeds but the partition cannot be read anymore. I suggest making `fileFormat` a final constructor parameter instead of a setter, and passing it from `FormatBatchWriteBuilder`. The public constructor can pass null. In that case, fail fast when a target partition has an explicit `file.format`, instead of comparing with null or silently skipping the update. Please also add a test that uses the public constructor to append and overwrite on such a partition. LGTM once this is fixed. -- 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]
