You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2024/11/05 16:22:51 UTC
[PR] Core, Puffin: Add DV file writer [iceberg]
aokolnychyi opened a new pull request, #11476:
URL: https://github.com/apache/iceberg/pull/11476
This PR adds a base DV file writer with basic tests. More tests to come once we support DV reads.
```
Benchmark (deletedRowsRatio) (referencedDataFileCount) Mode Cnt Score Error Units
DVWriterBenchmark.dv 0.01 5 ss 10 0.062 ± 0.003 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.01 5 ss 10 0.219 ± 0.017 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.01 5 ss 10 0.146 ± 0.006 s/op
DVWriterBenchmark.dv 0.01 10 ss 10 0.103 ± 0.010 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.01 10 ss 10 0.435 ± 0.016 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.01 10 ss 10 0.265 ± 0.014 s/op
DVWriterBenchmark.dv 0.03 5 ss 10 0.136 ± 0.006 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.03 5 ss 10 0.479 ± 0.018 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.03 5 ss 10 0.337 ± 0.018 s/op
DVWriterBenchmark.dv 0.03 10 ss 10 0.219 ± 0.013 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.03 10 ss 10 0.918 ± 0.028 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.03 10 ss 10 0.618 ± 0.014 s/op
DVWriterBenchmark.dv 0.05 5 ss 10 0.218 ± 0.014 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.05 5 ss 10 0.696 ± 0.010 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.05 5 ss 10 0.527 ± 0.008 s/op
DVWriterBenchmark.dv 0.05 10 ss 10 0.366 ± 0.011 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.05 10 ss 10 1.389 ± 0.024 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.05 10 ss 10 0.986 ± 0.007 s/op
DVWriterBenchmark.dv 0.10 5 ss 10 0.307 ± 0.007 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.10 5 ss 10 1.179 ± 0.039 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.10 5 ss 10 1.004 ± 0.010 s/op
DVWriterBenchmark.dv 0.10 10 ss 10 0.594 ± 0.006 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.10 10 ss 10 2.260 ± 0.039 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.10 10 ss 10 2.045 ± 0.054 s/op
DVWriterBenchmark.dv 0.2 5 ss 10 0.709 ± 0.011 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.2 5 ss 10 2.151 ± 0.017 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.2 5 ss 10 1.969 ± 0.046 s/op
DVWriterBenchmark.dv 0.2 10 ss 10 1.370 ± 0.047 s/op
DVWriterBenchmark.fileScopedParquetDeletes 0.2 10 ss 10 4.386 ± 0.034 s/op
DVWriterBenchmark.partitionScopedParquetDeletes 0.2 10 ss 10 3.849 ± 0.030 s/op
```
This work is part of #11122.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830590005
##########
core/src/main/java/org/apache/iceberg/deletes/DVFileWriter.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.deletes;
+
+import java.io.Closeable;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.DeleteWriteResult;
+
+/** A deletion vector file writer. */
+public interface DVFileWriter extends Closeable {
+ /**
+ * Marks a position in a given data file as deleted.
+ *
+ * @param path the data file path
+ * @param pos the data file position
+ * @param spec the data file partition spec
+ * @param partition the data file partition
+ */
+ void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition);
Review Comment:
In my view, `delete` makes sense now as previously we passed `PositionDelete` object and used the common writer abstraction. That said, I am OK either way.
I do want to keep the shorter class name. I use `DV` naming consistently throughout the code and it allows me to stay on one line most of the time. Our 100 character line limit is no joke!
--
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, Puffin: Add DV file writer [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830120314
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
I agree. It's probably a good idea to use String. We can always move to CharSequence later with minimal changes.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829669340
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -0,0 +1,128 @@
+/*
+ * 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.io;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.ParameterizedTestExtension;
+import org.apache.iceberg.Parameters;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.deletes.BaseDVFileWriter;
+import org.apache.iceberg.deletes.DVFileWriter;
+import org.apache.iceberg.deletes.PositionDeleteIndex;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.StructLikeSet;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestDVWriters<T> extends WriterTestBase<T> {
+
+ @Parameters(name = "formatVersion = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {3});
+ }
+
+ private OutputFileFactory fileFactory = null;
+
+ protected abstract StructLikeSet toSet(Iterable<T> records);
+
+ protected FileFormat dataFormat() {
+ return FileFormat.PARQUET;
+ }
+
+ @Override
+ @BeforeEach
+ public void setupTable() throws Exception {
+ this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(tableDir.delete()).isTrue(); // created during table creation
+
+ this.metadataDir = new File(tableDir, "metadata");
+ this.table = create(SCHEMA, PartitionSpec.unpartitioned());
+ this.fileFactory = OutputFileFactory.builderFor(table, 1, 1).format(FileFormat.PUFFIN).build();
+ }
+
+ @TestTemplate
+ public void testBasicDVs() throws IOException {
Review Comment:
I'll add more once we support DV reads.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830504286
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.io;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.ParameterizedTestExtension;
+import org.apache.iceberg.Parameters;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.deletes.BaseDVFileWriter;
+import org.apache.iceberg.deletes.DVFileWriter;
+import org.apache.iceberg.deletes.PositionDeleteIndex;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.StructLikeSet;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestDVWriters<T> extends WriterTestBase<T> {
+
+ @Parameters(name = "formatVersion = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {3});
+ }
+
+ private OutputFileFactory fileFactory = null;
+
+ protected abstract StructLikeSet toSet(Iterable<T> records);
+
+ protected FileFormat dataFormat() {
+ return FileFormat.PARQUET;
+ }
+
+ @Override
+ @BeforeEach
+ public void setupTable() throws Exception {
+ this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(tableDir.delete()).isTrue(); // created during table creation
+
Review Comment:
```suggestion
```
FYI this typically isn't needed, because `tableDir` is already initialized in the super class. See also https://github.com/apache/iceberg/pull/11460 where existing tests have been refactored to remove this unnecessary re-initialization
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#issuecomment-2460726490
Thanks for reviewing, @nastra @danielcweeks @jbonofre @rdblue!
--
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, Puffin: Add DV file writer [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830500893
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
+1 on using String, which I've also mentioned in https://github.com/apache/iceberg/pull/11302/files#r1824137648
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829658091
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
We need to discuss whether we should accept `String` or `CharSequence` in DV writers. We migrated to `String` for our new `DataFile` and `DeleteFile` fields but it is a different use case. We don't have to necessarily serialize these values. If an engine supports custom `CharSequence` objects, it may benefit from this API and skip the conversion.
It has no benefits for Spark, though. We have to convert to `String`.
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
Any thoughts?
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830494500
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
Moved the logic to the util and deprecated this class not to break anyone.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829761144
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
I'm not aware of any engine that would take advantage of it here, but I'm not sure it really causes any significant complexity to maintain as CharSequence. I'm fine either way.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830146472
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
+1 for `StructLikeUtil` or similar.
@jbonofre, the `io` classes typically refer to `FileIO` and buffer management utils in Iceberg. I'm not quite sure why this is currently in the `io` package. I think it's probably better to not continue using `io` so that it's easier to find later.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829672080
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
+ private final CharSequenceMap<BlobMetadata> blobsByPath = CharSequenceMap.create();
+ private DeleteWriteResult result = null;
+
+ public BaseDVFileWriter(
+ OutputFileFactory fileFactory,
+ Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes) {
+ this.fileFactory = fileFactory;
+ this.loadPreviousDeletes = loadPreviousDeletes;
+ }
+
+ @Override
+ public void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition) {
+ Deletes deletes = deletesByPath.computeIfAbsent(path, () -> new Deletes(path, spec, partition));
+ PositionDeleteIndex positions = deletes.positions();
+ positions.delete(pos);
+ }
+
+ @Override
+ public DeleteWriteResult result() {
+ Preconditions.checkState(result != null, "Cannot get result from unclosed writer");
+ return result;
+ }
+
+ @Override
+ public void close() throws IOException {
+ if (result == null) {
+ List<DeleteFile> deleteFiles = Lists.newArrayList();
+ CharSequenceSet referencedDataFiles = CharSequenceSet.empty();
+ List<DeleteFile> rewrittenDeleteFiles = Lists.newArrayList();
+
+ PuffinWriter writer = newWriter();
+
+ try (PuffinWriter closeableWriter = writer) {
+ for (Deletes deletes : deletesByPath.values()) {
+ CharSequence path = deletes.path();
+ PositionDeleteIndex positions = deletes.positions();
+ PositionDeleteIndex previousPositions = loadPreviousDeletes.apply(path);
+ if (previousPositions != null) {
+ positions.merge(previousPositions);
+ for (DeleteFile previousDeleteFile : previousPositions.deleteFiles()) {
+ // only file-scoped deletes can be discarded from the table state
+ if (ContentFileUtil.referencedDataFile(previousDeleteFile) != null) {
+ rewrittenDeleteFiles.add(previousDeleteFile);
+ }
+ }
+ }
+ write(closeableWriter, deletes);
+ referencedDataFiles.add(path);
+ }
+ }
+
+ // all delete files share the same Puffin path but have different content offsets
+ String puffinPath = writer.location();
+ long puffinFileSize = writer.fileSize();
+
+ for (CharSequence path : deletesByPath.keySet()) {
+ DeleteFile deleteFile = createDeleteFile(puffinPath, puffinFileSize, path);
+ deleteFiles.add(deleteFile);
+ }
+
+ this.result = new DeleteWriteResult(deleteFiles, referencedDataFiles, rewrittenDeleteFiles);
+ }
+ }
+
+ @SuppressWarnings("CollectionUndefinedEquality")
+ private DeleteFile createDeleteFile(String path, long size, CharSequence referencedDataFile) {
+ Deletes deletes = deletesByPath.get(referencedDataFile);
+ BlobMetadata blobMetadata = blobsByPath.get(referencedDataFile);
+ return FileMetadata.deleteFileBuilder(deletes.spec())
+ .ofPositionDeletes()
+ .withFormat(FileFormat.PUFFIN)
+ .withPath(path)
+ .withPartition(deletes.partition())
+ .withFileSizeInBytes(size)
+ .withReferencedDataFile(referencedDataFile)
+ .withContentOffset(blobMetadata.offset())
+ .withContentSizeInBytes(blobMetadata.length())
+ .withRecordCount(deletes.positions().cardinality())
+ .build();
+ }
+
+ private void write(PuffinWriter writer, Deletes deletes) {
+ CharSequence path = deletes.path();
+ PositionDeleteIndex positions = deletes.positions();
+ BlobMetadata blobMetadata = writer.write(toBlob(positions, path));
+ blobsByPath.put(path, blobMetadata);
+ }
+
+ private PuffinWriter newWriter() {
+ EncryptedOutputFile outputFile = fileFactory.newOutputFile();
+ String ident = "Iceberg " + IcebergBuild.fullVersion();
+ return Puffin.write(outputFile).createdBy(ident).build();
+ }
+
+ private Blob toBlob(PositionDeleteIndex positions, CharSequence path) {
+ return new Blob(
+ StandardBlobTypes.DV_V1,
+ ImmutableList.of(MetadataColumns.ROW_POSITION.fieldId()),
+ -1 /* snapshot ID is inherited */,
Review Comment:
As per V1 Puffin spec.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829991655
##########
core/src/main/java/org/apache/iceberg/deletes/DVFileWriter.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.deletes;
+
+import java.io.Closeable;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.DeleteWriteResult;
+
+/** A deletion vector file writer. */
+public interface DVFileWriter extends Closeable {
+ /**
+ * Marks a position in a given data file as deleted.
+ *
+ * @param path the data file path
+ * @param pos the data file position
+ * @param spec the data file partition spec
+ * @param partition the data file partition
+ */
+ void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition);
Review Comment:
Little bit of a nit here, but does `write` make sense? We're actually marking for delete. `FileAppender` and most "Writers" use `add`, should this just be `delete`?
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830015007
##########
core/src/main/java/org/apache/iceberg/deletes/DVFileWriter.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.deletes;
+
+import java.io.Closeable;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.DeleteWriteResult;
+
+/** A deletion vector file writer. */
+public interface DVFileWriter extends Closeable {
+ /**
+ * Marks a position in a given data file as deleted.
+ *
+ * @param path the data file path
+ * @param pos the data file position
+ * @param spec the data file partition spec
+ * @param partition the data file partition
+ */
+ void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition);
Review Comment:
Agree. What about `DVFileWriter` as the name? That seems OK but if there are alternatives, let me know.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830494978
##########
core/src/main/java/org/apache/iceberg/util/StructLikeUtil.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import org.apache.iceberg.StructLike;
+
+public class StructLikeUtil {
+
+ private StructLikeUtil() {}
+
+ public static StructLike copy(StructLike struct) {
+ return StructCopy.copy(struct);
+ }
+
+ private static class StructCopy implements StructLike {
Review Comment:
Copied the existing class but made private. The old one is deprecated and will be removed in 1.9.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#issuecomment-2851089832
> Hi team, may I ask a dummy question? I'm wondering if this is the official java implementation for deletion vector mentioned at https://iceberg.apache.org/puffin-spec/#deletion-vector-v1-blob-type? In other words, if I want to write deletion vector (in iceberg V3), should I use `BaseDVFileWriter`?
>
> My confusion lies in:
>
> * I see serialized blob should contain magic sequence "D1 D3 39 64", but I didn't find it anywhere
> * The spec mentions blob contains CRC32 for magic bytes and serialized bytes, I also don't find it
>
> Appreciate any answer! I could tell iceberg is evolving really fast, so as a newcomer I'm kinda lost in the (multiple) proposals and different versions of implementation.
@dentiny yes this repository contains the java implementation. You might want to start browsing the code around `BitmapPositionDeleteIndex` / `RoaringPositionBitmap` / `BaseDVFileWriter`, which should contain the info you're looking for.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi merged PR #11476:
URL: https://github.com/apache/iceberg/pull/11476
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829659538
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
I need to move this into a utility. Just not sure which at this point. Ideas are welcome.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830473765
##########
core/src/main/java/org/apache/iceberg/deletes/DVFileWriter.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.deletes;
+
+import java.io.Closeable;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.io.DeleteWriteResult;
+
+/** A deletion vector file writer. */
+public interface DVFileWriter extends Closeable {
+ /**
+ * Marks a position in a given data file as deleted.
+ *
+ * @param path the data file path
+ * @param pos the data file position
+ * @param spec the data file partition spec
+ * @param partition the data file partition
+ */
+ void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition);
Review Comment:
@danielcweeks from my standpoint, `delete()` would be confusing. As the class is named `DeleteVectorFileWriter` (yes, I would prefer a full name instead of `DV`), I think it makes sense to use `write()` there.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830471333
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
Good point. I like the `StructLikeUtil` name.
+1 for that. Thanks !
--
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, Puffin: Add DV file writer [iceberg]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829703181
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
What about a "generic" `IoUtil` ?
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
I think that `String` makes more sense for now because:
* to my knowledge, most of query engines support `String`
* if a query engine needs `CharSequence`, the conversion is not very painful
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829922422
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
Avro `Utf8` is the only example I remember.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1829993504
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
Does this even belong in `io`? This feels like it should be in `types.TypeUtil`
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830014006
##########
core/src/main/java/org/apache/iceberg/io/StructCopy.java:
##########
@@ -21,8 +21,8 @@
import org.apache.iceberg.StructLike;
/** Copy the StructLike's values into a new one. It does not handle list or map values now. */
-class StructCopy implements StructLike {
Review Comment:
I'd say it belongs to `StructLikeUtil` or something like this. Doesn't seem to belong to `io`.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#issuecomment-2833112453
Hi team, may I ask a dummy question?
I'm wondering if this is the official java implementation for deletion vector mentioned at https://iceberg.apache.org/puffin-spec/#deletion-vector-v1-blob-type?
My confusion lies in:
- I see serialized blob should contain magic sequence "D1 D3 39 64", but I didn't find it anywhere
- The spec mentions blob contains CRC32 for magic bytes and serialized bytes, I also don't find it
Appreciate any answer! I could tell iceberg is evolving really fast, so as a newcomer I'm kinda lost in the (multiple) proposals and different versions of 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, Puffin: Add DV file writer [iceberg]
Posted by "dentiny (via GitHub)" <gi...@apache.org>.
dentiny commented on PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#issuecomment-2914109123
> yes this repository contains the java implementation. You might want to start browsing the code around BitmapPositionDeleteIndex / RoaringPositionBitmap / BaseDVFileWriter, which should contain the info you're looking for.
Thank you for the reply and code pointer!
I did find crc and magic number in `BitmapPositionDeleteIndex`!
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1831651242
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.io;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.ParameterizedTestExtension;
+import org.apache.iceberg.Parameters;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.deletes.BaseDVFileWriter;
+import org.apache.iceberg.deletes.DVFileWriter;
+import org.apache.iceberg.deletes.PositionDeleteIndex;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.StructLikeSet;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestDVWriters<T> extends WriterTestBase<T> {
+
+ @Parameters(name = "formatVersion = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {3});
+ }
+
+ private OutputFileFactory fileFactory = null;
+
+ protected abstract StructLikeSet toSet(Iterable<T> records);
+
+ protected FileFormat dataFormat() {
+ return FileFormat.PARQUET;
+ }
+
+ @Override
+ @BeforeEach
+ public void setupTable() throws Exception {
+ this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(tableDir.delete()).isTrue(); // created during table creation
+
Review Comment:
Also removed the metadata dir and empty line.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830585211
##########
data/src/test/java/org/apache/iceberg/io/TestDVWriters.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * 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.io;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.ParameterizedTestExtension;
+import org.apache.iceberg.Parameters;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.data.BaseDeleteLoader;
+import org.apache.iceberg.data.DeleteLoader;
+import org.apache.iceberg.deletes.BaseDVFileWriter;
+import org.apache.iceberg.deletes.DVFileWriter;
+import org.apache.iceberg.deletes.PositionDeleteIndex;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.util.StructLikeSet;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestDVWriters<T> extends WriterTestBase<T> {
+
+ @Parameters(name = "formatVersion = {0}")
+ protected static List<Object> parameters() {
+ return Arrays.asList(new Object[] {3});
+ }
+
+ private OutputFileFactory fileFactory = null;
+
+ protected abstract StructLikeSet toSet(Iterable<T> records);
+
+ protected FileFormat dataFormat() {
+ return FileFormat.PARQUET;
+ }
+
+ @Override
+ @BeforeEach
+ public void setupTable() throws Exception {
+ this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+ assertThat(tableDir.delete()).isTrue(); // created during table creation
+
Review Comment:
Oh, good call. Missed that change!
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830583616
##########
core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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.deletes;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.iceberg.DeleteFile;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.FileMetadata;
+import org.apache.iceberg.IcebergBuild;
+import org.apache.iceberg.MetadataColumns;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.encryption.EncryptedOutputFile;
+import org.apache.iceberg.io.DeleteWriteResult;
+import org.apache.iceberg.io.OutputFileFactory;
+import org.apache.iceberg.io.StructCopy;
+import org.apache.iceberg.puffin.Blob;
+import org.apache.iceberg.puffin.BlobMetadata;
+import org.apache.iceberg.puffin.Puffin;
+import org.apache.iceberg.puffin.PuffinWriter;
+import org.apache.iceberg.puffin.StandardBlobTypes;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.util.CharSequenceMap;
+import org.apache.iceberg.util.CharSequenceSet;
+import org.apache.iceberg.util.ContentFileUtil;
+
+public class BaseDVFileWriter implements DVFileWriter {
+
+ private static final String REFERENCED_DATA_FILE_KEY = "referenced-data-file";
+ private static final String CARDINALITY_KEY = "cardinality";
+
+ private final OutputFileFactory fileFactory;
+ private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+ private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();
Review Comment:
Sounds good. Let's use `String`. It will have less overhead for engines like Spark that use `String`.
--
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, Puffin: Add DV file writer [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11476:
URL: https://github.com/apache/iceberg/pull/11476#discussion_r1830494978
##########
core/src/main/java/org/apache/iceberg/util/StructLikeUtil.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import org.apache.iceberg.StructLike;
+
+public class StructLikeUtil {
+
+ private StructLikeUtil() {}
+
+ public static StructLike copy(StructLike struct) {
+ return StructCopy.copy(struct);
+ }
+
+ private static class StructCopy implements StructLike {
Review Comment:
Copied the existing class but made private.
--
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