Core: Use avro compression properties from table properties while writing Manifest and Manifest list files. #5893

sumeetgajjar · 2022-09-30T00:15:45Z

This PR fixes #5892.

In this change, we add the table properties parameter to ManifestWriter. ManifestWriter further passes this properties map to the newAppender method, which uses the properties while creating AvroFileAppender.

Testing done:

Added a UT to validate the Avro compression properties are respected while writing the manifest and manifest list files.
Manually verified compression codec of manifest and manifest list files of an Iceberg Table (created using Spark) using avro-tools

sumeetgajjar · 2022-09-30T00:52:16Z

cc: @flyrain @rdblue @RussellSpitzer @amogh-jahagirdar @jackye1995

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

rdblue · 2022-10-04T16:21:45Z

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

+   * @return a manifest writer
+   */
+  public static ManifestWriter<DataFile> write(
+      PartitionSpec spec, OutputFile outputFile, Map<String, String> config) {


These methods should always include the format version.

Also, I don't think it is a good idea to add a property map throughout.

The write method without format version is already referenced in the code base prior to my changes.

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

Line 143 in 3170e6e

public static ManifestWriter<DataFile> write(PartitionSpec spec, OutputFile outputFile) {

Thus, I copied the signature of the above write method and added compressionCodec and compressionLevel as additional parameters.

Furthermore, if the caller decides to use the AVRO_COMPRESSION_LEVEL_DEFAULT value then using int as the datatype would not be possible.

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

Line 177 in 31edba2

public static final String AVRO_COMPRESSION_LEVEL_DEFAULT = null;

rdblue · 2022-10-04T16:27:57Z

@sumeetgajjar, I think it's a good idea to be able to set the compression codec, but 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. Let's pass the compression code explicitly.

…neric config map

sumeetgajjar · 2022-10-05T10:25:31Z

@sumeetgajjar, I think it's a good idea to be able to set the compression codec, but 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. Let's pass the compression code explicitly.

@rdblue thank you for the comment, incorporated the suggestions in the latest commit.

rdblue · 2022-10-06T23:14:05Z

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(


@sumeetgajjar, this must explicitly set the format version. This is a new method, so the format version should always be included. Older methods did not use a format version.

Adding formatVersion here would make its signature the same as the other newly added write method. Thus removing it altogether.

Removed the method altogether due to the reason stated above.

rdblue · 2022-10-06T23:15:40Z

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

+   * @return a manifest writer
+   */
+  public static ManifestWriter<DataFile> write(
+      PartitionSpec spec, OutputFile outputFile, String compressionCodec, String compressionLevel) {


Compression level is an int, not a string.

Sure, I'll refactor it to Integer.
The reason I suggest using Integer and not the primitive int type is to facilitate cases when the codec is set to snappy and uncompressed. Since both of these codecs, no compression level is required, null can be passed here.

I would like to hear your thoughts on this before I go ahead and make the change

I went ahead and used Integer for compressionLevel.
This is to ensure we can pass null values for Codecs that do not require it.
Had we used int, then we would not be able to represent null.

rdblue · 2022-10-06T23:18:53Z

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

+            table.spec(),
+            manifestLocation,
+            table.properties().get(TableProperties.AVRO_COMPRESSION),
+            table.properties().get(TableProperties.AVRO_COMPRESSION_LEVEL));


I don't think it is worth updating places like this. It's just needless code churn when we have no reason to think that the codec would be set or matter to the test. Can you remove cases like this from the PR?

Reverted and tried reducing the overall churn.

rdblue · 2022-10-06T23:19:18Z

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

+            parentId,
+            0,
+            /* compressionCodec */ null,
+            /* compressionLevel */ null)) {


Probably not worth updating this, can you revert it?

rdblue · 2022-10-06T23:21:25Z

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

@@ -22,6 +22,8 @@

 import java.io.IOException;
 import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.avro.AvroIterable;


What is the purpose of the updates here? I can understand testing that the manifest is written with a certain codec, but that should be in TestManifestWriter right? This tests different versions, which doesn't have much to do with the compression codec.

Sure, I'll move the ManifestWriter tests to TestManifestWriter.

For ManifestListWriter there does not exist a TestManifestListWriter class, I'll create the new test class and move the ManifestListerWriter tests to it.

rdblue

Thanks, @sumeetgajjar. This is looking better, but it has a lot of unnecessary changes just to call the new method with compression settings that don't matter. Can you make this smaller by removing the parts that don't need to be changed?

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

sumeetgajjar · 2022-10-07T07:05:18Z

Thanks, @sumeetgajjar. This is looking better, but it has a lot of unnecessary changes just to call the new method with compression settings that don't matter. Can you make this smaller by removing the parts that don't need to be changed?

Thanks @rdblue for the feedback, tried avoiding unnecessary changes and made the change comparatively smaller.

sumeetgajjar · 2022-10-31T20:45:20Z

@rdblue I have addressed your comments, can you please review the PR once you have time?

amogh-jahagirdar · 2022-11-03T04:12:55Z

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

+        if (compressionCodec != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION, compressionCodec);
+        }
+        if (compressionLevel != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION_LEVEL, compressionLevel.toString());
+        }


Nit: new line between the if blocks and the next statement

amogh-jahagirdar · 2022-11-03T04:13:14Z

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

+        if (compressionCodec != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION, compressionCodec);
+        }
+        if (compressionLevel != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION_LEVEL, compressionLevel.toString());
+        }


Same new line

amogh-jahagirdar · 2022-11-03T04:16:13Z

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

+        if (compressionCodec != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION, compressionCodec);
+        }
+        if (compressionLevel != null) {
+          builder.set(TableProperties.AVRO_COMPRESSION_LEVEL, compressionLevel.toString());
+        }


Nit: newline between blocks and the next statement

if (compressionCodec != null) { builder.set(TableProperties.AVRO_COMPRESSION, compressionCodec); } if (compressionLevel != null) { builder.set(TableProperties.AVRO_COMPRESSION_LEVEL, compressionLevel.toString()); } return builder.build();

amogh-jahagirdar · 2022-11-03T04:19:49Z

core/src/main/java/org/apache/iceberg/util/NumberUtil.java

+public class NumberUtil {
+
+  private NumberUtil() {}
+
+  /**
+   * @param value the string to convert, can be null
+   * @return parsed integer, returns null if the string is null
+   */
+  public static Integer createInteger(String value) {
+    if (value == null) {
+      return null;
+    }
+    return Integer.parseInt(value);
+  }


Do we actually need this or is there another util where the function can go? At minimum IMO shouldn't be public

nastra · 2022-11-09T10:12:53Z

core/src/main/java/org/apache/iceberg/util/NumberUtil.java

+   * @param value the string to convert, can be null
+   * @return parsed integer, returns null if the string is null
+   */
+  public static Integer createInteger(String value) {


I don't think we need this class. We are reading current.properties().get(TableProperties.AVRO_COMPRESSION_LEVEL) as a String in order to convert it to an Integer, just to convert it later back to a String

We don't need this class. Instead of NumberUtil.createInteger we can use PropertyUtil.propertyAsNullableInt on the properties Map and property name, and that gets the value of the property as an Integer.

nastra · 2022-11-09T10:15:33Z

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

+            manifestFile,
+            snapshotId,
+            compressionCodec,
+            /* compressionLevel */ null);


maybe just pass AVRO_COMPRESSION_LEVEL_DEFAULT rather than null?

nastra · 2022-11-09T10:16:22Z

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

@@ -237,12 +253,23 @@ ManifestFile writeManifest(DataFile... files) throws IOException {
  }

  ManifestFile writeManifest(Long snapshotId, DataFile... files) throws IOException {
-    File manifestFile = temp.newFile("input.m0.avro");


do we need to maintain/keep that input.m0.avro file name?

I'm pretty sure this is needed, because we're determining the file format from the file ending

Yes, we do need the manifest file to end in ".avro", but it's fine to call temp.newFile() (which uses a random name) and add the extension afterwards.

nastra · 2022-11-09T10:18:17Z

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

+
+  static final long SNAPSHOT_ID = 987134631982734L;
+
+  private static final long SEQUENCE_NUMBER = 34L;


do we need this constant? It seems to be used only in a single place

nastra · 2022-11-09T10:20:59Z

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

@@ -163,6 +166,19 @@ public class TableTestBase {

  static final FileIO FILE_IO = new TestTables.LocalFileIO();

+  static final Map<String, String> CODEC_METADATA_MAPPING =
+      ImmutableMap.<String, String>builder()
+          .put("uncompressed", "null")


can we use the Avro.Codec enum here rather than plain strings?

I don't see much benefit to using Avro.Codec. You'd have to get the name from the enum and then lowercase it. I think in this case, plain strings are fine.

nastra · 2022-11-09T10:23:21Z

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

+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class TestManifestListWriter extends TableTestBase {


what about adding these tests to TestManifestListVersions?

I think TestManifestListVersions is intended for tests of versions, similar to TestManifestWriterVersions. In fact, @rdblue suggested that the tests @sumeetgajjar added for manifests be moved from TestManifestWriterVersions to TestManifestWriter for that reason. There is no existing TestManifestListWriter, so Sumeet created one.

nastra · 2022-11-09T10:31:51Z

build.gradle

@@ -286,6 +286,8 @@ project(':iceberg-core') {
    testImplementation "org.xerial:sqlite-jdbc"
    testImplementation project(path: ':iceberg-api', configuration: 'testArtifacts')
    testImplementation "com.esotericsoftware:kryo"
+    testImplementation "org.xerial.snappy:snappy-java"


is this used anywhere?

It seems not and can be removed.

sumeetgajjar · 2023-01-09T22:30:05Z

Thanks for the reviews @rdblue @amogh-jahagirdar @nastra, really appreciate it.

There was an internal redistribution of tasks and a change of ownership of some GitHub PRs due to internal changes.
I am closing this PR and moving forward @wypoon or @sririshindra would be leading the changes suggested in this PR.
They would pull the public branch associated with this PR, add their commits on top of it and create a new PR to ease the logistics of the transition.

Thanks.

Respect avro compression properties for manifest files

bdf9d24

github-actions bot added build core flink spark labels Sep 30, 2022

sumeetgajjar added 2 commits September 29, 2022 17:20

Fix spotless java check

e4d9485

Fix stylecheck errors

4790f81

sumeetgajjar commented Sep 30, 2022

View reviewed changes

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

sumeetgajjar commented Sep 30, 2022

View reviewed changes

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

rdblue reviewed Oct 4, 2022

View reviewed changes

Explicitly pass compression codec and compression level instead of ge…

288458b

…neric config map

sumeetgajjar requested a review from rdblue October 5, 2022 15:39

rdblue reviewed Oct 6, 2022

View reviewed changes

rdblue requested changes Oct 6, 2022

View reviewed changes

Incorporate second round of feedback

93a3866

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

sumeetgajjar requested a review from rdblue October 7, 2022 07:05

amogh-jahagirdar reviewed Nov 3, 2022

View reviewed changes

nastra requested changes Nov 9, 2022

View reviewed changes

sumeetgajjar closed this Jan 9, 2023

wypoon mentioned this pull request Feb 10, 2023

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

Closed


		static final long SNAPSHOT_ID = 987134631982734L;

		private static final long SEQUENCE_NUMBER = 34L;

Core: Use avro compression properties from table properties while writing Manifest and Manifest list files. #5893

Core: Use avro compression properties from table properties while writing Manifest and Manifest list files. #5893

Conversation

sumeetgajjar commented Sep 30, 2022

sumeetgajjar commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Oct 4, 2022

sumeetgajjar commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumeetgajjar Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue left a comment

Choose a reason for hiding this comment

sumeetgajjar commented Oct 7, 2022

sumeetgajjar commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wypoon Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumeetgajjar commented Jan 9, 2023 • edited Loading

sumeetgajjar Oct 7, 2022 •

edited

Loading

wypoon Feb 10, 2023 •

edited

Loading

sumeetgajjar commented Jan 9, 2023 •

edited

Loading