Core: Use avro compression properties from table properties when writing manifests and manifest lists #6799

wypoon · 2023-02-10T02:01:21Z

This is a continuation of #5893.

Use the avro compression properties (write.avro.compression-codec and write.avro.compression-level) set in the table properties to determine compression when writing manifests and manifest lists for the table.

wypoon · 2023-02-10T16:58:10Z

I am picking up #5893 where @sumeetgajjar left off, as he is now pursuing other projects.
@rdblue I believe Sumeet has addressed your feedback. Compression level is an Integer, to allow for it to be null (for codecs that do not use compression level), and I have used PropertyUtil.propertyAsNullableInt to turn the String into an Integer. Sumeet has removed unnecessary changes from tests and only added new tests.
I have rebased Sumeet's changes on master, fixed some issues, and moved the Flink changes from 1.15 to 1.16 (the current default Flink version).
This is ready for review.
cc @nastra and @amogh-jahagirdar who reviewed the original PR as well.

core/src/test/java/org/apache/iceberg/TableTestBase.java

core/src/main/java/org/apache/iceberg/ManifestListWriter.java

.palantir/revapi.yml

core/src/test/java/org/apache/iceberg/TableTestBase.java

core/src/test/java/org/apache/iceberg/TestManifestListWriter.java

core/src/test/java/org/apache/iceberg/TestManifestWriter.java

versions.props

core/src/main/java/org/apache/iceberg/ManifestWriter.java

wypoon · 2023-02-15T22:40:41Z

@nastra, thank you for your reviews. I have addressed all your feedback.

core/src/test/java/org/apache/iceberg/TableTestBase.java

nastra

I believe the only thing left is fixing the breaking API change. @rdblue or @danielcweeks could you guys review this as well please?

core/src/test/java/org/apache/iceberg/TableTestBase.java

core/src/main/java/org/apache/iceberg/avro/Avro.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/main/java/org/apache/iceberg/ManifestLists.java

core/src/test/java/org/apache/iceberg/TableTestBase.java

core/src/test/java/org/apache/iceberg/TestManifestListWriter.java

versions.props

wypoon · 2023-02-23T18:57:59Z

@rdblue thank you for reviewing. I have just been on a short vacation. I'll look into your comments.

wypoon · 2023-02-24T06:41:45Z

@rdblue I have tried to address all your feedback. The only comment I did not understand is the one about not exposing Avro.Codec; I'm not sure what you'd like me to do. I'm fine with reverting it to private and simply using plain Strings in TestTableBase.AVRO_CODEC_NAME_MAPPING.

wypoon · 2023-03-08T19:39:03Z

@rdblue can you please review this again? @nastra has approved it.

ConeyLiu

+1 from my part

wypoon · 2023-09-21T18:32:22Z

TestStructuredStreamingRead3 > [catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default}] > testReadingStreamFromFutureTimetsamp[catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default}] FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting value to be true but was false
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at app//org.apache.iceberg.spark.source.TestStructuredStreamingRead3.lambda$testReadingStreamFromFutureTimetsamp$0(TestStructuredStreamingRead3.java:266)
        at [email protected]/java.util.stream.Streams$RangeIntSpliterator.forEachRemaining(Streams.java:104)
        at [email protected]/java.util.stream.IntPipeline$Head.forEach(IntPipeline.java:593)
        at app//org.apache.iceberg.spark.source.TestStructuredStreamingRead3.testReadingStreamFromFutureTimetsamp(TestStructuredStreamingRead3.java:263)

Is TestStructuredStreamingRead3 flaky?
spark-3x-scala-2-13-tests (8, 3.4) and spark-3x-java-17-tests (3.4, 2.13) passed; spark-3x-scala-2-13-tests (11, 3.4) failed due to this. It seems unlikely that it should fail just with java 11.
spark-3x-scala-2-13-tests (8, 3.5) and spark-3x-scala-2-13-tests (11, 3.5) were cancelled due to the above failure, but spark-3x-java-17-tests (3.5, 2.13) passed.

wypoon · 2023-09-22T00:24:57Z

@nastra @ConeyLiu thanks for reviewing. I have updated the PR and CI is green.
@ConeyLiu raises a good point about the new newAppender in ManifestWriter. I put up #8617 as an alternative. If there are no other sticking points, then I hope one of these two can be merged and make Iceberg 1.4.0.

wypoon · 2023-09-24T23:25:32Z

@nastra if you don't like #8617 can you please merge this instead? I have addressed all your feedback.

nastra · 2023-09-25T09:29:23Z

@nastra if you don't like #8617 can you please merge this instead? I have addressed all your feedback.

I think it would be good to get some additional review from one other committer. /cc @Fokko @amogh-jahagirdar @aokolnychyi could any of you review this one as well please?

aokolnychyi · 2023-10-12T01:32:02Z

Will do a review by the end of this week, sorry for the delay.

aokolnychyi · 2023-10-25T21:20:02Z

core/src/main/java/org/apache/iceberg/ManifestFiles.java

+   * @param compressionLevel compression level of the compressionCodec
+   * @return a manifest writer
+   */
+  public static ManifestWriter<DataFile> write(


I thought I already commented before going on vacation but can't seem to find the old discussion. Sorry if I post the same question again. Have we considered using a builder? My worry with the current approach was that we need to offer an overloaded method every time we add a new parameter.

@wypoon @nastra @rdblue?

@aokolnychyi thanks for reviewing. I'm interested to hear what @rdblue thinks. In the meantime, let me think about how to address your concern. However, using a builder will mean an API break, right?

@sumeetgajjar originally used a Map parameter for this , and @rdblue commented that "we don't want to pass a map of properties around. That's exposing too much where it doesn't need to be, and people tend to misuse generic arguments like this."
What I propose to do then is to introduce a ManifestWriter.Options class and use that here (instead of a Map). I'll also introduce a ManifestListWriter.Options class and use that in ManifestLists.write. These Options classes define what additional parameters are applicable and may be set. If in future, additional parameters are needed, they can be added to these Options classes.

wypoon · 2023-10-30T15:56:58Z

@nastra @aokolnychyi please let me know what you think. I updated the PR with the new approach.

…neric config map

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.

…evapi.

Make zstd-jni dependency testRuntimeOnly. Fix some nits. Simplify validate methods in TestManifestWriter and TestManifestListWriter.

... and have the static methods in ManifestFiles and ManifestLists for writing use them.

wypoon · 2023-11-30T05:19:54Z

Hmm, I think TestExpireSnapshotsAction > dataFilesCleanupWithParallelTasks might be a flaky test?
All Spark 3 tests passed with Java 8 and 11, and even for Java 17, they passed for Scala 2.13, so it seems unlikely that there is a problem just with Scala 2.12 on Java 17.

wypoon · 2023-11-30T06:56:34Z

@nastra @aokolnychyi I have rebased on main and resolved the conflicts with the RewriteManifestsSparkAction refactoring. As I mentioned before, I have introduced ManifestWriter.Options and ManifestListWriter.Options to be passed to ManifestFiles.write and ManifestLists.write. These Options classes provide defined options (currently just compression codec and level but extensible in future) that may be passed.

github-actions · 2024-08-25T00:15:06Z

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.

github-actions · 2024-09-01T00:16:56Z

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.

github-actions bot added build core flink spark labels Feb 10, 2023

nastra self-requested a review February 10, 2023 07:35

wypoon commented Feb 10, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/TableTestBase.java Outdated Show resolved Hide resolved

nastra reviewed Feb 14, 2023

View reviewed changes

wypoon commented Feb 15, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/ManifestWriter.java Outdated Show resolved Hide resolved

wypoon force-pushed the avro_manifest_compression branch 2 times, most recently from f5457a3 to c81b209 Compare February 15, 2023 22:30

nastra reviewed Feb 16, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/TableTestBase.java Show resolved Hide resolved

nastra reviewed Feb 16, 2023

View reviewed changes