Core: Use avro compression properties from table properties when writing manifests and manifest lists (API change) #8617
Conversation
1. remove unecessary edits to use compression codec and compression level from tests 2. move ManifestWriter tests to TestManifestWriter 3. move ManifestListWriter tests to TestManifestListWriter 4. remove unwanted `ManifestLists#write` method
Remove NumberUtil. Use PropertyUtil.propertyAsNullableInt instead. Add convenience method propertyAsNullableInt to TableMetadata. Fix a TestTableBase#writeManifest to write a file with .avro extension. Revert Flink 1.15 changes and make them in Flink 1.16 instead.
Retain but deprecate old newAppender method in ManifestWriter. Make Codec public in Avro, we can can use the enum values. Rename CODEC_METADATA_MAPPING to AVRO_CODEC_NAME_MAPPING in TableTestBase and provide explanatory comment. Use named constants in the map. Adopt suggestion to use AssertJ assertions in test verification. Update zstd-jni version.
Make zstd-jni dependency testRuntimeOnly. Fix some nits. Simplify validate methods in TestManifestWriter and TestManifestListWriter.
| - code: "java.class.removed" | ||
| old: "interface org.apache.iceberg.actions.RewritePositionDeleteStrategy" | ||
| justification: "Removing deprecated code" | ||
| - code: "java.method.abstractMethodAdded" |
There was a problem hiding this comment.
we shouldn't be breaking the API. See also my comment in #6799 (comment) on how to avoid breaking the API for adding the new method
There was a problem hiding this comment.
Yes, your suggestion was followed in #6799 and I haven't reverted it there.
I think @ConeyLiu's point is that the new newAppender method doesn't break the API because it is not abstract -- it calls the old newAppender. However, the old newAppender is deprecated and when it is removed, the new newAppender then has to be changed. There is no natural implementation for it, so it should be abstract. Of course, one could do something else, such as return null or throw an UnsupportedOperationException.
If anyone out there actually writes their own ManifestWriter subclass, they would need to implement a newAppender(PartitionSpec, OutputFile, String, Integer) method. We avoid breaking the API now, which allows them to get away without implementing the new method, but when newAppender(PartitionSpec, OutputFile) is then removed, that will break them (even if we do not technically break the API by keeping newAppender(PartitionSpec, OutputFile, String, Integer) non-abstract and throwing an UnsupportedOperationException -- it effectively breaks them).
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
3 participants
Variant of #6799 except that new
newAppendermethod added toManifestWriteris declared abstract.This means that it is a breaking API change.