Core, Puffin: Add DV file writer #11476

aokolnychyi · 2024-11-05T16:22:48Z

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.

aokolnychyi · 2024-11-05T16:27:19Z

core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java

+
+  private final OutputFileFactory fileFactory;
+  private final Function<CharSequence, PositionDeleteIndex> loadPreviousDeletes;
+  private final CharSequenceMap<Deletes> deletesByPath = CharSequenceMap.create();


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.

Any thoughts?

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

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.

Avro Utf8 is the only example I remember.

I agree. It's probably a good idea to use String. We can always move to CharSequence later with minimal changes.

+1 on using String, which I've also mentioned in https://github.com/apache/iceberg/pull/11302/files#r1824137648

Sounds good. Let's use String. It will have less overhead for engines like Spark that use String.

aokolnychyi · 2024-11-05T16:28:11Z

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 {


I need to move this into a utility. Just not sure which at this point. Ideas are welcome.

What about a "generic" IoUtil ?

Does this even belong in io? This feels like it should be in types.TypeUtil

I'd say it belongs to StructLikeUtil or something like this. Doesn't seem to belong to io.

+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.

Good point. I like the StructLikeUtil name.

+1 for that. Thanks !

Moved the logic to the util and deprecated this class not to break anyone.

aokolnychyi · 2024-11-05T16:34:37Z

data/src/test/java/org/apache/iceberg/io/TestDVWriters.java

+  }
+
+  @TestTemplate
+  public void testBasicDVs() throws IOException {


I'll add more once we support DV reads.

aokolnychyi · 2024-11-05T16:36:29Z

core/src/main/java/org/apache/iceberg/deletes/BaseDVFileWriter.java

+    return new Blob(
+        StandardBlobTypes.DV_V1,
+        ImmutableList.of(MetadataColumns.ROW_POSITION.fieldId()),
+        -1 /* snapshot ID is inherited */,


As per V1 Puffin spec.

danielcweeks · 2024-11-05T20:57:26Z

core/src/main/java/org/apache/iceberg/deletes/DVFileWriter.java

+   * @param spec the data file partition spec
+   * @param partition the data file partition
+   */
+  void write(CharSequence path, long pos, PartitionSpec spec, StructLike partition);


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?

Agree. What about DVFileWriter as the name? That seems OK but if there are alternatives, let me know.

@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.

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!

aokolnychyi · 2024-11-06T07:13:13Z

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

+    return StructCopy.copy(struct);
+  }
+
+  private static class StructCopy implements StructLike {


Copied the existing class but made private. The old one is deprecated and will be removed in 1.9.

nastra · 2024-11-06T07:23:14Z

data/src/test/java/org/apache/iceberg/io/TestDVWriters.java

+    this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
+    assertThat(tableDir.delete()).isTrue(); // created during table creation
+


Suggested change

this.tableDir = Files.createTempDirectory(temp, "junit").toFile();

assertThat(tableDir.delete()).isTrue(); // created during table creation

FYI this typically isn't needed, because tableDir is already initialized in the super class. See also #11460 where existing tests have been refactored to remove this unnecessary re-initialization

Oh, good call. Missed that change!

Also removed the metadata dir and empty line.

aokolnychyi · 2024-11-06T20:35:10Z

Thanks for reviewing, @nastra @danielcweeks @jbonofre @rdblue!

github-actions bot added spark core data labels Nov 5, 2024

aokolnychyi commented Nov 5, 2024

View reviewed changes

danielcweeks reviewed Nov 5, 2024

View reviewed changes

aokolnychyi added 2 commits November 5, 2024 22:36

Core, Puffin: Add DV file writer

8845eb6

Improve

e95cf2c

aokolnychyi force-pushed the dv-writer branch from 52c0f37 to e95cf2c Compare November 6, 2024 07:10

aokolnychyi commented Nov 6, 2024

View reviewed changes

nastra reviewed Nov 6, 2024

View reviewed changes

nastra approved these changes Nov 6, 2024

View reviewed changes

Feedback

89b3107

danielcweeks approved these changes Nov 6, 2024

View reviewed changes

aokolnychyi merged commit 7938403 into apache:main Nov 6, 2024
49 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core, Puffin: Add DV file writer #11476

Core, Puffin: Add DV file writer #11476

aokolnychyi commented Nov 5, 2024

aokolnychyi Nov 5, 2024

aokolnychyi Nov 5, 2024

jbonofre Nov 5, 2024

danielcweeks Nov 5, 2024

aokolnychyi Nov 5, 2024

rdblue Nov 5, 2024

nastra Nov 6, 2024

aokolnychyi Nov 6, 2024

aokolnychyi Nov 5, 2024

jbonofre Nov 5, 2024

danielcweeks Nov 5, 2024

aokolnychyi Nov 5, 2024

rdblue Nov 5, 2024

jbonofre Nov 6, 2024

aokolnychyi Nov 6, 2024

aokolnychyi Nov 5, 2024

aokolnychyi Nov 5, 2024

danielcweeks Nov 5, 2024

aokolnychyi Nov 5, 2024

jbonofre Nov 6, 2024

aokolnychyi Nov 6, 2024

aokolnychyi Nov 6, 2024 •

edited

Loading

nastra Nov 6, 2024

aokolnychyi Nov 6, 2024

aokolnychyi Nov 6, 2024

aokolnychyi commented Nov 6, 2024

		this.tableDir = Files.createTempDirectory(temp, "junit").toFile();
		assertThat(tableDir.delete()).isTrue(); // created during table creation

Core, Puffin: Add DV file writer #11476

Core, Puffin: Add DV file writer #11476

Conversation

aokolnychyi commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aokolnychyi commented Nov 6, 2024

aokolnychyi Nov 6, 2024 •

edited

Loading