You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "RussellSpitzer (via GitHub)" <gi...@apache.org> on 2026/03/16 14:47:15 UTC
[PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
RussellSpitzer opened a new pull request, #15652:
URL: https://github.com/apache/iceberg/pull/15652
## Summary
- Adds `writerProperties` support to `ManifestWriter` and `ManifestFiles` factory methods, allowing configuration to be passed through to the underlying Avro writer for all format versions (V1–V4).
- `SnapshotProducer` now reads `write.avro.compression-codec` and `write.avro.compression-level` from table properties and forwards them when writing manifests, enabling per-table manifest compression configuration.
- Includes tests verifying default (deflate) and custom (snappy) compression across all format versions, and a JMH benchmark (`ManifestBenchmark`) for comparing codec write/read performance.
## Test plan
- [x] `testDefaultManifestCompression` — verifies all format versions produce manifests with `deflate` codec by default
- [x] `testCustomManifestCompression` — verifies setting `write.avro.compression-codec=snappy` produces manifests with `snappy` codec for all format versions
- [ ] Run full `TestManifestWriterVersions` suite to confirm no regressions
- [ ] Run `ManifestBenchmark` JMH benchmark across codecs to validate performance characteristics
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944838485
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = Map.of(spec.specId(), spec);
+ this.writerProperties = Map.of(TableProperties.AVRO_COMPRESSION, codec);
+ this.numEntries = entriesForColumnCount(numCols);
+ this.dataFiles = generateDataFiles();
+ setupReadManifest();
+ }
+
+ @Setup(Level.Invocation)
+ public void setupWriteInvocation() throws IOException {
+ this.writeBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-write-").toAbsolutePath().toString();
+ this.writeOutputFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", writeBaseDir));
+
+ for (DataFile file : dataFiles) {
+ file.path();
+ file.fileSizeInBytes();
+ file.recordCount();
+ }
+ }
+
+ @TearDown(Level.Trial)
+ public void tearDownTrial() {
+ cleanDir(readBaseDir);
+ readBaseDir = null;
+ readManifest = null;
+ dataFiles = null;
+ }
+
+ @TearDown(Level.Invocation)
+ public void tearDownInvocation() {
+ cleanDir(writeBaseDir);
+ writeBaseDir = null;
+ writeOutputFile = null;
+ }
+
+ @AuxCounters(AuxCounters.Type.EVENTS)
+ @State(Scope.Thread)
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ public static class FileSizeCounters {
+ public double manifestSizeMB;
+
+ @Setup(Level.Invocation)
+ public void reset() {
+ manifestSizeMB = 0;
+ }
+ }
+
+ @Benchmark
+ @Threads(1)
+ public ManifestFile writeManifest(FileSizeCounters counters) throws IOException {
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, writeOutputFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ }
+
+ ManifestFile manifest = writer.toManifestFile();
+ counters.manifestSizeMB = manifest.length() / (1024.0 * 1024.0);
+ return manifest;
+ }
+
+ @Benchmark
+ @Threads(1)
+ public void readManifest(Blackhole blackhole) throws IOException {
+ TestTables.LocalFileIO fileIO = new TestTables.LocalFileIO();
+ try (CloseableIterator<DataFile> it =
+ ManifestFiles.read(readManifest, fileIO, specsById).iterator()) {
+ while (it.hasNext()) {
+ blackhole.consume(it.next());
+ }
+ }
+ }
+
+ private void setupReadManifest() {
+ try {
+ this.readBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-read-").toAbsolutePath().toString();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ OutputFile manifestFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", readBaseDir));
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, manifestFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ this.readManifest = writer.toManifestFile();
+ }
+
+ private List<DataFile> generateDataFiles() {
+ Random random = new Random(42);
+ List<DataFile> files = Lists.newArrayListWithCapacity(numEntries);
+ for (int i = 0; i < numEntries; i++) {
+ DataFiles.Builder builder =
+ DataFiles.builder(spec)
+ .withFormat(FileFormat.PARQUET)
+ .withPath(String.format(Locale.ROOT, "/path/to/data-%d.parquet", i))
+ .withFileSizeInBytes(1024 + i)
+ .withRecordCount(1000 + i)
+ .withMetrics(randomMetrics(random, numCols));
+
+ if (!spec.isUnpartitioned()) {
+ builder.withPartitionPath(
+ String.format(
+ Locale.ROOT, "id=%d/data=val-%d/customer=cust-%d", i % 100, i % 50, i % 200));
+ }
+
+ files.add(builder.build());
+ }
+ return files;
Review Comment:
nit: newline
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944847832
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = Map.of(spec.specId(), spec);
+ this.writerProperties = Map.of(TableProperties.AVRO_COMPRESSION, codec);
+ this.numEntries = entriesForColumnCount(numCols);
+ this.dataFiles = generateDataFiles();
+ setupReadManifest();
+ }
+
+ @Setup(Level.Invocation)
+ public void setupWriteInvocation() throws IOException {
+ this.writeBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-write-").toAbsolutePath().toString();
+ this.writeOutputFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", writeBaseDir));
+
+ for (DataFile file : dataFiles) {
+ file.path();
+ file.fileSizeInBytes();
+ file.recordCount();
+ }
+ }
+
+ @TearDown(Level.Trial)
+ public void tearDownTrial() {
+ cleanDir(readBaseDir);
+ readBaseDir = null;
+ readManifest = null;
+ dataFiles = null;
+ }
+
+ @TearDown(Level.Invocation)
+ public void tearDownInvocation() {
+ cleanDir(writeBaseDir);
+ writeBaseDir = null;
+ writeOutputFile = null;
+ }
+
+ @AuxCounters(AuxCounters.Type.EVENTS)
+ @State(Scope.Thread)
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ public static class FileSizeCounters {
+ public double manifestSizeMB;
+
+ @Setup(Level.Invocation)
+ public void reset() {
+ manifestSizeMB = 0;
+ }
+ }
+
+ @Benchmark
+ @Threads(1)
+ public ManifestFile writeManifest(FileSizeCounters counters) throws IOException {
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, writeOutputFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ }
+
+ ManifestFile manifest = writer.toManifestFile();
+ counters.manifestSizeMB = manifest.length() / (1024.0 * 1024.0);
+ return manifest;
+ }
+
+ @Benchmark
+ @Threads(1)
+ public void readManifest(Blackhole blackhole) throws IOException {
+ TestTables.LocalFileIO fileIO = new TestTables.LocalFileIO();
+ try (CloseableIterator<DataFile> it =
+ ManifestFiles.read(readManifest, fileIO, specsById).iterator()) {
+ while (it.hasNext()) {
+ blackhole.consume(it.next());
+ }
+ }
+ }
+
+ private void setupReadManifest() {
+ try {
+ this.readBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-read-").toAbsolutePath().toString();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ OutputFile manifestFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", readBaseDir));
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, manifestFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ this.readManifest = writer.toManifestFile();
+ }
+
+ private List<DataFile> generateDataFiles() {
+ Random random = new Random(42);
+ List<DataFile> files = Lists.newArrayListWithCapacity(numEntries);
+ for (int i = 0; i < numEntries; i++) {
+ DataFiles.Builder builder =
+ DataFiles.builder(spec)
+ .withFormat(FileFormat.PARQUET)
+ .withPath(String.format(Locale.ROOT, "/path/to/data-%d.parquet", i))
+ .withFileSizeInBytes(1024 + i)
+ .withRecordCount(1000 + i)
+ .withMetrics(randomMetrics(random, numCols));
+
+ if (!spec.isUnpartitioned()) {
+ builder.withPartitionPath(
+ String.format(
+ Locale.ROOT, "id=%d/data=val-%d/customer=cust-%d", i % 100, i % 50, i % 200));
+ }
+
+ files.add(builder.build());
+ }
+ return files;
+ }
+
+ static int entriesForColumnCount(int cols) {
+ return ENTRY_BASE / cols;
+ }
+
+ static Metrics randomMetrics(Random random, int cols) {
+ long rowCount = 100_000L + random.nextInt(1000);
+ Map<Integer, Long> columnSizes = Maps.newHashMap();
+ Map<Integer, Long> valueCounts = Maps.newHashMap();
+ Map<Integer, Long> nullValueCounts = Maps.newHashMap();
+ Map<Integer, Long> nanValueCounts = Maps.newHashMap();
+ Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap();
+ Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap();
+ for (int i = 0; i < cols; i++) {
+ columnSizes.put(i, 1_000_000L + random.nextInt(100_000));
+ valueCounts.put(i, 100_000L + random.nextInt(100));
+ nullValueCounts.put(i, (long) random.nextInt(5));
+ nanValueCounts.put(i, (long) random.nextInt(5));
+ byte[] lower = new byte[8];
+ random.nextBytes(lower);
+ lowerBounds.put(i, ByteBuffer.wrap(lower));
+ byte[] upper = new byte[8];
+ random.nextBytes(upper);
+ upperBounds.put(i, ByteBuffer.wrap(upper));
+ }
+
+ return new Metrics(
+ rowCount,
+ columnSizes,
+ valueCounts,
+ nullValueCounts,
+ nanValueCounts,
+ lowerBounds,
+ upperBounds);
+ }
+
+ private static void cleanDir(String dir) {
+ if (dir != null) {
+ FileUtils.deleteQuietly(new java.io.File(dir));
Review Comment:
import?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4068518632
As a note for anyone checking those benchmarks, the only really compressible information we are adding to the manifests in these tests are the paths and the counts. The "min and max" for each column are just completely random and that's most of the manifest.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #15652:
URL: https://github.com/apache/iceberg/pull/15652
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4099621241
Merged! Thanks @danielcweeks , @pvary for the review!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2942352308
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,25 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.MANIFEST_COMPRESSION, TableProperties.MANIFEST_COMPRESSION_DEFAULT);
+ builder.put(TableProperties.AVRO_COMPRESSION, codec);
Review Comment:
This will have to be format specific when we add in Parquet as well
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4068468325
<img width="1635" height="326" alt="image" src="https://github.com/user-attachments/assets/56c70107-e98a-4d2e-bf5c-436ff8325976" />
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940944749
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -241,6 +260,55 @@ static ManifestWriter<DataFile> newWriter(
"Cannot write manifest for table version: " + formatVersion);
}
+ /**
+ * Create a new {@link ManifestWriter} for the given format version with custom writer properties.
+ *
+ * @param formatVersion a target format version
+ * @param spec a {@link PartitionSpec}
+ * @param outputFile an {@link OutputFile} where the manifest will be written
+ * @param snapshotId a snapshot ID for the manifest entries, or null for an inherited ID
+ * @param writerProperties properties passed through to the underlying file writer
+ * @return a manifest writer
+ */
+ public static ManifestWriter<DataFile> write(
+ int formatVersion,
+ PartitionSpec spec,
+ OutputFile outputFile,
+ Long snapshotId,
+ Map<String, String> writerProperties) {
+ return newWriter(
+ formatVersion,
+ spec,
+ EncryptedFiles.plainAsEncryptedOutput(outputFile),
+ snapshotId,
+ null,
+ writerProperties);
+ }
+
+ @VisibleForTesting
+ static ManifestWriter<DataFile> newWriter(
Review Comment:
Why the old `newWriter` does not call this with empty `writerProperties`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940953435
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -253,16 +280,20 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
Review Comment:
This is for the V4 writer. Did we want Parquet here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940974669
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
Are we sure it is a good idea to reuse the same property for the manifest compression?
Seems like a bit awkward to set Avro properties on a Parquet table.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4068434729
> Nice idea @RussellSpitzer! Maybe we could even default to snappy? Maybe the next graph should contain the sizes in a different chart, so it is easier to see not just the cost, but the gains as well 😄
Great Idea, I kept trying to figure out how to get that info in and another graph makes sense. I'll regenerate it
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944830564
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = Map.of(spec.specId(), spec);
+ this.writerProperties = Map.of(TableProperties.AVRO_COMPRESSION, codec);
+ this.numEntries = entriesForColumnCount(numCols);
+ this.dataFiles = generateDataFiles();
+ setupReadManifest();
+ }
+
+ @Setup(Level.Invocation)
+ public void setupWriteInvocation() throws IOException {
+ this.writeBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-write-").toAbsolutePath().toString();
+ this.writeOutputFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", writeBaseDir));
+
+ for (DataFile file : dataFiles) {
+ file.path();
+ file.fileSizeInBytes();
+ file.recordCount();
+ }
+ }
+
+ @TearDown(Level.Trial)
+ public void tearDownTrial() {
+ cleanDir(readBaseDir);
+ readBaseDir = null;
+ readManifest = null;
+ dataFiles = null;
+ }
+
+ @TearDown(Level.Invocation)
+ public void tearDownInvocation() {
+ cleanDir(writeBaseDir);
+ writeBaseDir = null;
+ writeOutputFile = null;
+ }
+
+ @AuxCounters(AuxCounters.Type.EVENTS)
+ @State(Scope.Thread)
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ public static class FileSizeCounters {
+ public double manifestSizeMB;
+
+ @Setup(Level.Invocation)
+ public void reset() {
+ manifestSizeMB = 0;
+ }
+ }
+
+ @Benchmark
+ @Threads(1)
+ public ManifestFile writeManifest(FileSizeCounters counters) throws IOException {
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, writeOutputFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ }
+
+ ManifestFile manifest = writer.toManifestFile();
+ counters.manifestSizeMB = manifest.length() / (1024.0 * 1024.0);
+ return manifest;
+ }
+
+ @Benchmark
+ @Threads(1)
+ public void readManifest(Blackhole blackhole) throws IOException {
+ TestTables.LocalFileIO fileIO = new TestTables.LocalFileIO();
+ try (CloseableIterator<DataFile> it =
+ ManifestFiles.read(readManifest, fileIO, specsById).iterator()) {
+ while (it.hasNext()) {
+ blackhole.consume(it.next());
+ }
+ }
+ }
+
+ private void setupReadManifest() {
+ try {
+ this.readBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-read-").toAbsolutePath().toString();
Review Comment:
could we import?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944862106
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -276,6 +335,30 @@ public static ManifestWriter<DeleteFile> writeDeleteManifest(
formatVersion, spec, EncryptedFiles.plainAsEncryptedOutput(outputFile), snapshotId);
}
+ /**
+ * Create a new {@link ManifestWriter} for the given format version with custom writer properties.
+ *
+ * @param formatVersion a target format version
+ * @param spec a {@link PartitionSpec}
+ * @param outputFile an {@link OutputFile} where the manifest will be written
+ * @param snapshotId a snapshot ID for the manifest entries, or null for an inherited ID
+ * @param writerProperties properties passed through to the underlying file writer
+ * @return a manifest writer
+ */
+ public static ManifestWriter<DeleteFile> writeDeleteManifest(
Review Comment:
Same question about the deprecation
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941020005
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
We don't have Parquet manifests (yet) in this PR. When we add that I think we should respect either the Avro or Parquet property depending on the manifest output format.
My broader question is wether we should have a manifest specific compression property but as I mentioned in the discussion, I don't think it really makes sense to have a user use one compression type for data files and a different for manifests ... but maybe it does? it would be easy enough for us to add later if the use case comes up.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941948684
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
I'm a little concerned that we're using the data file default for metadata.
Why wouldn't we introduce a separate metadata compression and level that applies to all metadata files? Do we need to be format specific about this anyway? Seems like we could just have a single config and pass that through to the writers regardless (the codecs are generally supported across all the formats, I believe)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945906978
##########
core/src/test/java/org/apache/iceberg/TestManifestWriterVersions.java:
##########
@@ -313,6 +317,48 @@ public void testV3ManifestRewriteWithInheritance() throws IOException {
checkRewrittenEntry(readManifest(manifest3), 0L, FileContent.DATA, FIRST_ROW_ID);
}
+ @ParameterizedTest
+ @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
+ public void testDefaultManifestCompression(int formatVersion) throws IOException {
+ File manifestFile = temp.resolve("default-v" + formatVersion + ".avro").toFile();
+ OutputFile outputFile = Files.localOutput(manifestFile);
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID);
+ try {
+ writer.add(DATA_FILE);
+ } finally {
+ writer.close();
+ }
+
+ assertThat(readAvroCodec(manifestFile)).isEqualTo("deflate");
+ }
+
+ @ParameterizedTest
+ @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
+ public void testCustomManifestCompression(int formatVersion) throws IOException {
+ Map<String, String> props = ImmutableMap.of(TableProperties.AVRO_COMPRESSION, "snappy");
+ File manifestFile = temp.resolve("snappy-v" + formatVersion + ".avro").toFile();
+ OutputFile outputFile = Files.localOutput(manifestFile);
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID, props);
+ try {
+ writer.add(DATA_FILE);
+ } finally {
+ writer.close();
+ }
+
+ assertThat(readAvroCodec(manifestFile)).isEqualTo("snappy");
+ }
+
+ private static String readAvroCodec(File file) throws IOException {
Review Comment:
Maybe move it to some util like `AvroTestHelpers`? This is not public ATM, so it is a bit questionable 😄
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941238378
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
Could be, we currently have these properties
Property | Default
-- | --
write.avro.compression-codec | gzip
write.delete.avro.compression-codec | (same)
write.avro.compression-level | null
write.delete.avro.compression-level | null
You think we should just add another
write.avro.manifest.compression-codec
or leave it format independent since we may have a
write.manifest.format
and then
write.manifest.compression-codec?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944845222
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = Map.of(spec.specId(), spec);
+ this.writerProperties = Map.of(TableProperties.AVRO_COMPRESSION, codec);
+ this.numEntries = entriesForColumnCount(numCols);
+ this.dataFiles = generateDataFiles();
+ setupReadManifest();
+ }
+
+ @Setup(Level.Invocation)
+ public void setupWriteInvocation() throws IOException {
+ this.writeBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-write-").toAbsolutePath().toString();
+ this.writeOutputFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", writeBaseDir));
+
+ for (DataFile file : dataFiles) {
+ file.path();
+ file.fileSizeInBytes();
+ file.recordCount();
+ }
+ }
+
+ @TearDown(Level.Trial)
+ public void tearDownTrial() {
+ cleanDir(readBaseDir);
+ readBaseDir = null;
+ readManifest = null;
+ dataFiles = null;
+ }
+
+ @TearDown(Level.Invocation)
+ public void tearDownInvocation() {
+ cleanDir(writeBaseDir);
+ writeBaseDir = null;
+ writeOutputFile = null;
+ }
+
+ @AuxCounters(AuxCounters.Type.EVENTS)
+ @State(Scope.Thread)
+ @SuppressWarnings("checkstyle:VisibilityModifier")
+ public static class FileSizeCounters {
+ public double manifestSizeMB;
+
+ @Setup(Level.Invocation)
+ public void reset() {
+ manifestSizeMB = 0;
+ }
+ }
+
+ @Benchmark
+ @Threads(1)
+ public ManifestFile writeManifest(FileSizeCounters counters) throws IOException {
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, writeOutputFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ }
+
+ ManifestFile manifest = writer.toManifestFile();
+ counters.manifestSizeMB = manifest.length() / (1024.0 * 1024.0);
+ return manifest;
+ }
+
+ @Benchmark
+ @Threads(1)
+ public void readManifest(Blackhole blackhole) throws IOException {
+ TestTables.LocalFileIO fileIO = new TestTables.LocalFileIO();
+ try (CloseableIterator<DataFile> it =
+ ManifestFiles.read(readManifest, fileIO, specsById).iterator()) {
+ while (it.hasNext()) {
+ blackhole.consume(it.next());
+ }
+ }
+ }
+
+ private void setupReadManifest() {
+ try {
+ this.readBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-read-").toAbsolutePath().toString();
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ OutputFile manifestFile =
+ org.apache.iceberg.Files.localOutput(
+ String.format(Locale.ROOT, "%s/manifest.avro", readBaseDir));
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(FORMAT_VERSION, spec, manifestFile, 1L, writerProperties);
+
+ try (ManifestWriter<DataFile> w = writer) {
+ for (DataFile file : dataFiles) {
+ w.add(file);
+ }
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+
+ this.readManifest = writer.toManifestFile();
+ }
+
+ private List<DataFile> generateDataFiles() {
+ Random random = new Random(42);
+ List<DataFile> files = Lists.newArrayListWithCapacity(numEntries);
+ for (int i = 0; i < numEntries; i++) {
+ DataFiles.Builder builder =
+ DataFiles.builder(spec)
+ .withFormat(FileFormat.PARQUET)
+ .withPath(String.format(Locale.ROOT, "/path/to/data-%d.parquet", i))
+ .withFileSizeInBytes(1024 + i)
+ .withRecordCount(1000 + i)
+ .withMetrics(randomMetrics(random, numCols));
+
+ if (!spec.isUnpartitioned()) {
+ builder.withPartitionPath(
+ String.format(
+ Locale.ROOT, "id=%d/data=val-%d/customer=cust-%d", i % 100, i % 50, i % 200));
+ }
+
+ files.add(builder.build());
+ }
+ return files;
+ }
+
+ static int entriesForColumnCount(int cols) {
+ return ENTRY_BASE / cols;
+ }
Review Comment:
Do we need this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944849381
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = Map.of(spec.specId(), spec);
+ this.writerProperties = Map.of(TableProperties.AVRO_COMPRESSION, codec);
+ this.numEntries = entriesForColumnCount(numCols);
+ this.dataFiles = generateDataFiles();
+ setupReadManifest();
+ }
+
+ @Setup(Level.Invocation)
+ public void setupWriteInvocation() throws IOException {
+ this.writeBaseDir =
+ java.nio.file.Files.createTempDirectory("bench-write-").toAbsolutePath().toString();
+ this.writeOutputFile =
+ org.apache.iceberg.Files.localOutput(
Review Comment:
At least import one of the Files
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945295471
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -288,16 +310,18 @@ protected FileAppender<ManifestEntry<DeleteFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "deletes")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "4")
+ .meta("content", "deletes")
+ .overwrite();
+ builder.set(writerProperties());
+ return builder.build();
Review Comment:
```suggestion
return InternalData.write(FileFormat.AVRO, file)
.schema(manifestSchema)
.named("manifest_entry")
.meta("schema", SchemaParser.toJson(spec.schema()))
.meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
.meta("partition-spec-id", String.valueOf(spec.specId()))
.meta("format-version", "4")
.meta("content", "deletes")
.set(writerProperties())
.overwrite();
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944863069
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -287,15 +370,34 @@ public static ManifestWriter<DeleteFile> writeDeleteManifest(
*/
public static ManifestWriter<DeleteFile> writeDeleteManifest(
int formatVersion, PartitionSpec spec, EncryptedOutputFile outputFile, Long snapshotId) {
+ return writeDeleteManifest(formatVersion, spec, outputFile, snapshotId, Collections.emptyMap());
Review Comment:
Same question about the `Map.of()`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941022369
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
See #15634 for parquet manifest writing
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4068376239
Nice idea @RussellSpitzer!
Maybe we could even default to snappy?
Maybe the next graph should contain the sizes in a different chart, so it is easier to see not just the cost, but the gains as well 😄
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2942265101
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
I think that's a good solution, https://github.com/apache/iceberg/pull/15652#discussion_r2941238378 I have the pr ready now i"ll push in a few seconds
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941106484
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
I think having different config value would make sense.
Given the number of times we read the manifest, and the different characteristics I would choose differently in cases where I need every IO/CPU to optimize my query.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944860568
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -211,6 +212,25 @@ public static ManifestWriter<DataFile> write(
return newWriter(formatVersion, spec, encryptedOutputFile, snapshotId, null);
}
+ /**
+ * Create a new {@link ManifestWriter} for the given format version with custom writer properties.
+ *
+ * @param formatVersion a target format version
+ * @param spec a {@link PartitionSpec}
+ * @param encryptedOutputFile an {@link EncryptedOutputFile} where the manifest will be written
+ * @param snapshotId a snapshot ID for the manifest entries, or null for an inherited ID
+ * @param writerProperties properties passed through to the underlying file writer
+ * @return a manifest writer
+ */
+ public static ManifestWriter<DataFile> write(
Review Comment:
Really just a question. Do we want to keep all of the `write` versions? Shall we deprecate the old ones?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#issuecomment-4086591605
Any other comments @danielcweeks ?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945880850
##########
core/src/test/java/org/apache/iceberg/TestManifestWriterVersions.java:
##########
@@ -313,6 +317,48 @@ public void testV3ManifestRewriteWithInheritance() throws IOException {
checkRewrittenEntry(readManifest(manifest3), 0L, FileContent.DATA, FIRST_ROW_ID);
}
+ @ParameterizedTest
+ @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
+ public void testDefaultManifestCompression(int formatVersion) throws IOException {
+ File manifestFile = temp.resolve("default-v" + formatVersion + ".avro").toFile();
+ OutputFile outputFile = Files.localOutput(manifestFile);
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID);
+ try {
+ writer.add(DATA_FILE);
+ } finally {
+ writer.close();
+ }
+
+ assertThat(readAvroCodec(manifestFile)).isEqualTo("deflate");
+ }
+
+ @ParameterizedTest
+ @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
+ public void testCustomManifestCompression(int formatVersion) throws IOException {
+ Map<String, String> props = ImmutableMap.of(TableProperties.AVRO_COMPRESSION, "snappy");
+ File manifestFile = temp.resolve("snappy-v" + formatVersion + ".avro").toFile();
+ OutputFile outputFile = Files.localOutput(manifestFile);
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID, props);
+ try {
Review Comment:
try-with-resources
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2941948684
##########
core/src/main/java/org/apache/iceberg/SnapshotProducer.java:
##########
@@ -625,6 +628,24 @@ protected RollingManifestWriter<DeleteFile> newRollingDeleteManifestWriter(Parti
() -> newDeleteManifestWriter(spec), targetManifestSizeBytes);
}
+ private static Map<String, String> manifestWriterProperties(TableMetadata metadata) {
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+
+ String codec =
+ metadata.property(
+ TableProperties.AVRO_COMPRESSION, TableProperties.AVRO_COMPRESSION_DEFAULT);
Review Comment:
I'm a little concerned that we're using the data file settings for metadata.
Why wouldn't we introduce a separate metadata compression and level that applies to all metadata files? Do we need to be format specific about this anyway? Seems like we could just have a single config and pass that through to the writers regardless (the codecs are generally supported across all the formats, I believe)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940982185
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -253,16 +280,20 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "4")
+ .meta("content", "data")
+ .overwrite();
+ for (Map.Entry<String, String> entry : writerProperties().entrySet()) {
+ builder.set(entry.getKey(), entry.getValue());
+ }
Review Comment:
That's probably a good choice, I'll add it in.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940956001
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -253,16 +280,20 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "4")
+ .meta("content", "data")
+ .overwrite();
+ for (Map.Entry<String, String> entry : writerProperties().entrySet()) {
+ builder.set(entry.getKey(), entry.getValue());
+ }
Review Comment:
Shall we add a `InternalData.WriteBuilder.set(Map)` instead with a default implementation?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940998396
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -241,6 +260,55 @@ static ManifestWriter<DataFile> newWriter(
"Cannot write manifest for table version: " + formatVersion);
}
+ /**
+ * Create a new {@link ManifestWriter} for the given format version with custom writer properties.
+ *
+ * @param formatVersion a target format version
+ * @param spec a {@link PartitionSpec}
+ * @param outputFile an {@link OutputFile} where the manifest will be written
+ * @param snapshotId a snapshot ID for the manifest entries, or null for an inherited ID
+ * @param writerProperties properties passed through to the underlying file writer
+ * @return a manifest writer
+ */
+ public static ManifestWriter<DataFile> write(
+ int formatVersion,
+ PartitionSpec spec,
+ OutputFile outputFile,
+ Long snapshotId,
+ Map<String, String> writerProperties) {
+ return newWriter(
+ formatVersion,
+ spec,
+ EncryptedFiles.plainAsEncryptedOutput(outputFile),
+ snapshotId,
+ null,
+ writerProperties);
+ }
+
+ @VisibleForTesting
+ static ManifestWriter<DataFile> newWriter(
Review Comment:
Good call, that should be a bit tighter.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940979074
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -253,16 +280,20 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
Review Comment:
The V4 writing Parquet Pr is in #15634 And requires a bunch more changes. This PR just handles passing through compression codec so we don't change the format to parquet
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2944855275
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -227,15 +247,54 @@ static ManifestWriter<DataFile> newWriter(
EncryptedOutputFile encryptedOutputFile,
Long snapshotId,
Long firstRowId) {
+ return newWriter(
+ formatVersion, spec, encryptedOutputFile, snapshotId, firstRowId, Collections.emptyMap());
Review Comment:
Why not `Map.of()`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945879179
##########
core/src/test/java/org/apache/iceberg/TestManifestWriterVersions.java:
##########
@@ -313,6 +317,48 @@ public void testV3ManifestRewriteWithInheritance() throws IOException {
checkRewrittenEntry(readManifest(manifest3), 0L, FileContent.DATA, FIRST_ROW_ID);
}
+ @ParameterizedTest
+ @FieldSource("org.apache.iceberg.TestHelpers#ALL_VERSIONS")
+ public void testDefaultManifestCompression(int formatVersion) throws IOException {
+ File manifestFile = temp.resolve("default-v" + formatVersion + ".avro").toFile();
+ OutputFile outputFile = Files.localOutput(manifestFile);
+
+ ManifestWriter<DataFile> writer =
+ ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID);
+ try {
+ writer.add(DATA_FILE);
+ } finally {
+ writer.close();
+ }
Review Comment:
Can we use try-with-resources?
```suggestion
try (ManifestWriter<DataFile> writer =
ManifestFiles.write(formatVersion, SPEC, outputFile, SNAPSHOT_ID)) {
writer.add(DATA_FILE);
}
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945295471
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -288,16 +310,18 @@ protected FileAppender<ManifestEntry<DeleteFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V4Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "4")
- .meta("content", "deletes")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "4")
+ .meta("content", "deletes")
+ .overwrite();
+ builder.set(writerProperties());
+ return builder.build();
Review Comment:
```suggestion
return InternalData.write(FileFormat.AVRO, file)
.schema(manifestSchema)
.named("manifest_entry")
.meta("schema", SchemaParser.toJson(spec.schema()))
.meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
.meta("partition-spec-id", String.valueOf(spec.specId()))
.meta("format-version", "4")
.meta("content", "deletes")
.set(writerProperties())
.overwrite();
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2945306344
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -328,16 +357,18 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V3Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "3")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "3")
+ .meta("content", "data")
+ .overwrite();
+ builder.set(writerProperties());
+ return builder.build();
Review Comment:
```suggestion
return InternalData.write(FileFormat.AVRO, file)
.schema(manifestSchema)
.named("manifest_entry")
.meta("schema", SchemaParser.toJson(spec.schema()))
.meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
.meta("partition-spec-id", String.valueOf(spec.specId()))
.meta("format-version", "3")
.meta("content", "deletes")
.set(writerProperties())
.overwrite()
.build();
```
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -328,16 +357,18 @@ protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
Schema manifestSchema = V3Metadata.entrySchema(spec.partitionType());
try {
- return InternalData.write(FileFormat.AVRO, file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "3")
- .meta("content", "data")
- .overwrite()
- .build();
+ InternalData.WriteBuilder builder =
+ InternalData.write(FileFormat.AVRO, file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "3")
+ .meta("content", "data")
+ .overwrite();
+ builder.set(writerProperties());
+ return builder.build();
Review Comment:
This could be used everywhere
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940923352
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmark.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.io.UncheckedIOException;
+import java.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import org.apache.commons.io.FileUtils;
+import org.apache.iceberg.io.CloseableIterator;
+import org.apache.iceberg.io.OutputFile;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.types.Types;
+import org.openjdk.jmh.annotations.AuxCounters;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Timeout;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+/**
+ * A benchmark that measures manifest read/write performance across compression codecs.
+ *
+ * <p>Entry counts are calibrated per column count via {@link #ENTRY_BASE}. Set to 300_000 for ~8 MB
+ * manifests (matching the default {@code commit.manifest.target-size-bytes}) or 15_000 for ~400 KB.
+ *
+ * <p>To run this benchmark:
+ *
+ * <pre>{@code
+ * # all combinations
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark
+ *
+ * # single codec
+ * ./gradlew :iceberg-core:jmh -PjmhIncludeRegex=ManifestBenchmark \
+ * -PjmhParams="codec=gzip"
+ * }</pre>
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 6)
+@Measurement(iterations = 10)
+@BenchmarkMode(Mode.SingleShotTime)
+@Timeout(time = 10, timeUnit = TimeUnit.MINUTES)
+public class ManifestBenchmark {
+
+ static final int ENTRY_BASE = 300_000;
+
+ private static final int FORMAT_VERSION = 4;
+
+ private static final Schema SCHEMA =
+ new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "data", Types.StringType.get()),
+ Types.NestedField.required(3, "customer", Types.StringType.get()));
+
+ private static final PartitionSpec SPEC =
+ PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ @Param({"gzip", "snappy", "zstd", "uncompressed"})
+ private String codec;
+
+ @Param({"true", "false"})
+ private String partitioned;
+
+ @Param({"10", "50", "100"})
+ private int numCols;
+
+ private PartitionSpec spec;
+ private Map<Integer, PartitionSpec> specsById;
+ private Map<String, String> writerProperties;
+ private List<DataFile> dataFiles;
+ private int numEntries;
+
+ private String writeBaseDir;
+ private OutputFile writeOutputFile;
+
+ private String readBaseDir;
+ private ManifestFile readManifest;
+
+ @Setup(Level.Trial)
+ public void setupTrial() {
+ this.spec = Boolean.parseBoolean(partitioned) ? SPEC : PartitionSpec.unpartitioned();
+ this.specsById = ImmutableMap.of(spec.specId(), spec);
Review Comment:
I think we can now use the builtin `Map.of()` which I believe creates an immutable map and is a bit cleaner.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Propagate Avro compression settings to manifest writers [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15652:
URL: https://github.com/apache/iceberg/pull/15652#discussion_r2940944749
##########
core/src/main/java/org/apache/iceberg/ManifestFiles.java:
##########
@@ -241,6 +260,55 @@ static ManifestWriter<DataFile> newWriter(
"Cannot write manifest for table version: " + formatVersion);
}
+ /**
+ * Create a new {@link ManifestWriter} for the given format version with custom writer properties.
+ *
+ * @param formatVersion a target format version
+ * @param spec a {@link PartitionSpec}
+ * @param outputFile an {@link OutputFile} where the manifest will be written
+ * @param snapshotId a snapshot ID for the manifest entries, or null for an inherited ID
+ * @param writerProperties properties passed through to the underlying file writer
+ * @return a manifest writer
+ */
+ public static ManifestWriter<DataFile> write(
+ int formatVersion,
+ PartitionSpec spec,
+ OutputFile outputFile,
+ Long snapshotId,
+ Map<String, String> writerProperties) {
+ return newWriter(
+ formatVersion,
+ spec,
+ EncryptedFiles.plainAsEncryptedOutput(outputFile),
+ snapshotId,
+ null,
+ writerProperties);
+ }
+
+ @VisibleForTesting
+ static ManifestWriter<DataFile> newWriter(
Review Comment:
Why the old `newWriter` does not call this with null `writerProperties`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org