stevenzwu commented on code in PR #15634:
URL: https://github.com/apache/iceberg/pull/15634#discussion_r3103637597
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -65,7 +67,8 @@ private ManifestWriter(
Long snapshotId,
Long firstRowId,
Map<String, String> writerProperties) {
- this.file = file.encryptingOutputFile();
+ this.format =
FileFormat.fromFileName(file.encryptingOutputFile().location());
Review Comment:
should the file format be passed in as an arg?
##########
core/src/main/java/org/apache/iceberg/V4Metadata.java:
##########
@@ -278,28 +279,35 @@ static Schema wrapFileSchema(Types.StructType fileSchema)
{
}
static Types.StructType fileType(Types.StructType partitionType) {
- return Types.StructType.of(
- DataFile.CONTENT.asRequired(),
- DataFile.FILE_PATH,
- DataFile.FILE_FORMAT,
- required(
- DataFile.PARTITION_ID, DataFile.PARTITION_NAME, partitionType,
DataFile.PARTITION_DOC),
- DataFile.RECORD_COUNT,
- DataFile.FILE_SIZE,
- DataFile.COLUMN_SIZES,
- DataFile.VALUE_COUNTS,
- DataFile.NULL_VALUE_COUNTS,
- DataFile.NAN_VALUE_COUNTS,
- DataFile.LOWER_BOUNDS,
- DataFile.UPPER_BOUNDS,
- DataFile.KEY_METADATA,
- DataFile.SPLIT_OFFSETS,
- DataFile.EQUALITY_IDS,
- DataFile.SORT_ORDER_ID,
- DataFile.FIRST_ROW_ID,
- DataFile.REFERENCED_DATA_FILE,
- DataFile.CONTENT_OFFSET,
- DataFile.CONTENT_SIZE);
+ List<Types.NestedField> fields = Lists.newArrayList();
+ fields.add(DataFile.CONTENT.asRequired());
+ fields.add(DataFile.FILE_PATH);
+ fields.add(DataFile.FILE_FORMAT);
+ if (!partitionType.fields().isEmpty()) {
Review Comment:
can we make this field optional and write null instead? I am asking because
the `adjust` position in the V4Metadata irks me a bit.
##########
core/src/main/java/org/apache/iceberg/BaseFile.java:
##########
@@ -329,7 +330,9 @@ protected <T> void internalSet(int pos, T value) {
this.partitionSpecId = (value != null) ? (Integer) value : -1;
return;
case 4:
- this.partitionData = (PartitionData) value;
+ if (value != null) {
Review Comment:
nit: `value != null ? (PartitionData) value : null`
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -240,6 +248,18 @@ public ManifestFile toManifestFile() {
firstRowId);
}
+ private ByteBuffer keyMetadataBuffer() {
+ if (keyMetadata instanceof NativeEncryptionKeyMetadata nativeKeyMetadata
Review Comment:
The `NativeEncryptionKeyMetadata` puts me off a bit maybe due to the name
`Native`. I thought only Parquet file format has native encryption. Avro files
use the generic AES GCM encryption.
the logic seems the same as the old code. So this is good.
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -82,6 +85,20 @@ private ManifestWriter(
protected abstract FileAppender<ManifestEntry<F>> newAppender(
PartitionSpec spec, OutputFile outputFile);
+ private OutputFile outputFile(EncryptedOutputFile encryptedFile) {
+ // Casting to NativeEncryptionOutputFile actually makes the file rely on
native encryption
+ // rather than whole-file encryption.
+ if (format == FileFormat.PARQUET
+ && encryptedFile instanceof NativeEncryptionOutputFile nativeFile) {
+ return nativeFile;
+ }
+ return encryptedFile.encryptingOutputFile();
Review Comment:
nit: missing empty line after control block in a few places
##########
core/src/jmh/java/org/apache/iceberg/ManifestBenchmarkUtil.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * 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.nio.ByteBuffer;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Random;
+import org.apache.commons.io.FileUtils;
+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;
+
+/**
+ * Shared constants and stateless helpers for {@link ManifestBenchmark} and
{@link
+ * ManifestCompressionBenchmark}.
+ */
+final class ManifestBenchmarkUtil {
+
+ 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()));
+
+ static final PartitionSpec SPEC =
+
PartitionSpec.builderFor(SCHEMA).identity("id").identity("data").identity("customer").build();
+
+ private ManifestBenchmarkUtil() {}
+
+ /**
+ * Returns the number of manifest entries for the given column count. The
result is {@code
+ * entryBase / cols}.
+ *
+ * <p>The linear ratio was determined empirically by writing manifests at
various column counts
+ * and measuring the resulting file sizes. An {@code entryBase} of 300,000
produces ~8 MB
+ * manifests (matching the default {@code
commit.manifest.target-size-bytes}); 15,000 produces
+ * ~400 KB.
+ */
+ static int entriesForColumnCount(int entryBase, int cols) {
+ return entryBase / cols;
+ }
+
+ static List<DataFile> generateDataFiles(PartitionSpec spec, int numEntries,
int numCols) {
+ 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()) {
Review Comment:
nit: `isPartitioned`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
