nastra commented on code in PR #5893: URL: https://github.com/apache/iceberg/pull/5893#discussion_r1017734825
########## core/src/main/java/org/apache/iceberg/util/NumberUtil.java: ########## @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.util; + +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) { Review Comment: 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 ########## 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") + .put("zstd", "zstandard") + .put("gzip", "deflate") + .build(); + + static final String AVRO_CODEC_KEY = "avro.codec"; + + static final long SNAPSHOT_ID = 987134631982734L; + + private static final long SEQUENCE_NUMBER = 34L; Review Comment: do we need this constant? It seems to be used only in a single place ########## core/src/test/java/org/apache/iceberg/TableTestBase.java: ########## @@ -293,11 +320,24 @@ <F extends ContentFile<F>> ManifestFile writeManifest( ManifestFile writeDeleteManifest(int newFormatVersion, Long snapshotId, DeleteFile... deleteFiles) throws IOException { + return writeDeleteManifest( + newFormatVersion, snapshotId, /* compressionCodec */ null, deleteFiles); + } + + ManifestFile writeDeleteManifest( + int newFormatVersion, Long snapshotId, String compressionCodec, DeleteFile... deleteFiles) + throws IOException { OutputFile manifestFile = org.apache.iceberg.Files.localOutput( FileFormat.AVRO.addExtension(temp.newFile().toString())); ManifestWriter<DeleteFile> writer = - ManifestFiles.writeDeleteManifest(newFormatVersion, SPEC, manifestFile, snapshotId); + ManifestFiles.writeDeleteManifest( + newFormatVersion, + SPEC, + manifestFile, + snapshotId, + compressionCodec, + /* compressionLevel */ null); Review Comment: maybe just pass `AVRO_COMPRESSION_LEVEL_DEFAULT` rather than null? ########## 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"); Review Comment: do we need to maintain/keep that `input.m0.avro` file name? ########## core/src/test/java/org/apache/iceberg/TestManifestListWriter.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg; + +import java.io.IOException; +import java.util.Map; +import org.apache.iceberg.avro.AvroIterable; +import org.apache.iceberg.io.InputFile; +import org.hamcrest.Matchers; +import org.junit.Assert; +import org.junit.Assume; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class TestManifestListWriter extends TableTestBase { Review Comment: what about adding these tests to `TestManifestListVersions`? ########## 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"); Review Comment: I'm pretty sure this is needed, because we're determining the file format from the file ending ########## 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" Review Comment: is this used anywhere? ########## 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") Review Comment: can we use the `Avro.Codec` enum here rather than plain strings? -- 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: issues-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
