Core: Add position deletes metadata table by szehon-ho · Pull Request #6365 · apache/iceberg

szehon-ho · 2022-12-06T04:52:47Z

This breaks up the pr #4812 , and is just the part to add the table PositionDeletesTable.

It is now based on @aokolnychyi 's newly-added BatchScan interface (#5922), added for this purpose so the scan is free to not return FileScanTask. It returns a custom ScanTask that scan DeleteFiles rather than DataFiles.

szehon-ho · 2022-12-07T18:41:04Z

  static PartitionSpec transformSpec(Schema metadataTableSchema, PartitionSpec spec) {
    PartitionSpec.Builder identitySpecBuilder =
-        PartitionSpec.builderFor(metadataTableSchema).checkConflicts(false);
+        PartitionSpec.builderFor(metadataTableSchema)


Before this change, predicate pushdown would make the PositionDeletes scan tasks have wrong partition field id, spec id, so they will not work in the DeleteFile read.

Though it only happens in corner cases like dropped partition fields (where the auto-generated field-ids are not correct anymore). Added a test for this in TestMetadataTableScansWithPartitionEvolution

RussellSpitzer · 2022-12-19T18:42:10Z

          SPEC_ID.fieldId(),
-          PARTITION_COLUMN_ID);
+          PARTITION_COLUMN_ID,
+          POSITION_DELETE_TABLE_PARTITION_FIELD_ID,


Why do we need a Position_Delete version of Spec_ID and File_Path, shouldn't we be able to use the original metadata columns for these?

Yea its true, we could do that. I think I was going back and forth whether we wanted to have that or not, as these are 'main' columns with a proper name, versus hidden columns (whose name start with _), in other words they are not the exact same column. Im open

Do we even have to make them metadata columns then? I thought they would be just regular columns in a table. I don't think they should be added to META_COLUMNS. I think metadata columns should be only about columns we can project on demand. That's why we did not add changelog columns here.

Let me also think about reserving field IDs for them. It is similar yet different use case compared to changelog columns as there is no changelog table as such.

Removed from list of metadata columns. It was from an earlier change where I was trying to re-use existing Spark RowReader and it checked that projected column must either be part of schema, or metadata column.

Kept the field ids in this file to reserve them, to avoid conflict with "row" struct of this table, which has the data table schema.

RussellSpitzer · 2022-12-20T01:04:10Z

  }

+  @Test
+  public void testBasicSplitPlanningDeleteFiles() {


Do we store split offsets of Delete files? If so should we be checking the splitting on those boundaries?

I think not but could be wrong, I dont see it being set in DeleteFile builder: https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/FileMetadata.java#L38

Hum, I guess we have decided they will be smaller than data files so I guess it doesn't matter.

I don't see a reason why we wouldn't store that. I think it was overlooked.
@szehon-ho, can we do that in a follow-up PR? Not urgent, at least we should create an issue.

Made: #6659

RussellSpitzer

I think this is really close, I have a few remaining questions but just minor issues

aokolnychyi · 2022-12-20T02:31:13Z

I'll have time to take a look tomorrow too.

aokolnychyi · 2022-12-20T16:30:06Z

          SPEC_ID.fieldId(),
-          PARTITION_COLUMN_ID);
+          PARTITION_COLUMN_ID,
+          POSITION_DELETE_TABLE_PARTITION_FIELD_ID,


Do we even have to make them metadata columns then? I thought they would be just regular columns in a table. I don't think they should be added to META_COLUMNS. I think metadata columns should be only about columns we can project on demand. That's why we did not add changelog columns here.

Let me also think about reserving field IDs for them. It is similar yet different use case compared to changelog columns as there is no changelog table as such.

aokolnychyi · 2022-12-20T16:37:16Z

+
+  public static class PositionDeletesTableScan
+      extends AbstractTableScan<
+          BatchScan, org.apache.iceberg.ScanTask, ScanTaskGroup<org.apache.iceberg.ScanTask>>


Can we give ScanTask in this class a more specific name and then avoid this qualified import?

aokolnychyi · 2022-12-20T16:47:24Z

+    this.table = table;
+  }
+
+  protected Table table() {


Do we need a method?

Removed this, as I inherit now from BaseMetadataTable.

szehon-ho · 2022-12-21T18:54:31Z

+                      PartitionSpec spec = transformedSpecs.get(entry.file().specId());
+                      String specString = PartitionSpecParser.toJson(spec);
+                      return new PositionDeleteScanTask(
+                          entry.file().copy(),


I spent some time on this. But if we iterate through the tasks without making a copy here, it corrupts the older tasks already iterated through (setting them all their file values to the latest tasks's!). See TestMetadataTableScans::testPositionDeletesUnpartitioned and run without this copy.

It looks like it's because the underlying AvroIterable reuses containers. So here I add copy() to avoid this problem. Maybe I can use copyWithoutStats()?

That would be the correct thing to do here, if you see ManifestEntries.java that's how it works because you need the full list of tasks not just an iterator of them

To be clear, you need copyWithoutStats here since you aren't using the metrics past this point. Just to save a bit of memory

I think your other option is to just not return a parallel itterable and rely on callers to know they need to copy

Thanks, yea it was something in the ParallelIterable that messed it up. Anyway I changed to copyWithoutStats, it'll make it easier for the caller. Definitely stumped on this for half a day, glad I added the extra test though and saw this.

aokolnychyi

I did another round. I'll need to check transformSpec with fresh eyes. Getting close, though. Thanks, @szehon-ho!

aokolnychyi · 2022-12-22T22:37:08Z

          Types.LongType.get(),
          "Commit snapshot ID");

+  public static final int POSITION_DELETE_TABLE_PARTITION_FIELD_ID = Integer.MAX_VALUE - 107;


If I understand correctly, the table schema will include these 3 columns in addition to columns in delete files. It is not bad to reserve some IDs but have we thought about keeping the table schema limited to the content of delete files and supporting already existing _spec_id, _partition, _file metadata columns? Values for metadata columns will be only projected on demand, just like we can do that for regular tables.

It seems cleaner to me and shouldn't be hard to do since we will have a dedicated reader.

Thinking one this a little bit, one problem is, this table is partitioned by "partition", and if I remember correctly there's some complication in partitioning by a hidden metadata column. So ended up making all of them actual columns..

aokolnychyi · 2022-12-22T23:30:24Z

+              m -> {
+                // Filter partitions
+                CloseableIterable<ManifestEntry<DeleteFile>> deleteFileEntries =
+                    ManifestFiles.readDeleteManifest(m, tableOps().io(), transformedSpecs)


Do we need to pass a projection while reading delete manifests?

I assume you mean, with/without stats? Fixed. Took me a little bit to figure it out, I needed to add a DELETE_SCAN_COLUMNS here that has content, versus the base scan's SCAN_COLUMNS which does not. As I have a filter on position_delete file content.

szehon-ho · 2023-01-05T03:23:53Z

+  protected static final List<String> DELETE_SCAN_COLUMNS =
+      ImmutableList.of(
+          "snapshot_id",
+          "content",


This has to differ from data file's SCAN_COLUMNS by including content, which is used in the scan to filter later.

szehon-ho · 2023-01-10T01:32:48Z

Rebased and squash commits

aokolnychyi

Almost there!

aokolnychyi · 2023-01-20T20:38:44Z

  private MetadataColumns() {}

  // IDs Integer.MAX_VALUE - (1-100) are used for metadata columns
+  public static final int FILE_PATH_COLUMN_ID = Integer.MAX_VALUE - 1;


+1 to this solution

aokolnychyi · 2023-01-20T20:51:54Z

+            Types.NestedField.optional(
+                MetadataColumns.DELETE_FILE_ROW_FIELD_ID,
+                "row",
+                table().schema().asStruct(),


Once we add support to engines, we will have to test schema evolution.

Yea, good point, will need to check this.

szehon-ho · 2023-01-21T06:21:05Z

+
+    List<ScanTask> tasks = Lists.newArrayList(scan.planFiles());
+
+    Assert.assertEquals(


Added new tests for ScanMetrics. These cover number of manifests skipped and read. Somejhow the existing ManifestReader code we invoke does not update metrics about number of deleteFiles skipped and read (at least in the code path we use): https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ManifestReader.java#L228

Made: #6658

aokolnychyi

LGTM. Great work, @szehon-ho! I left some optional comments. Feel free to merge whenever you are ready.

aokolnychyi · 2023-01-24T04:22:40Z

  }

+  @Test
+  public void testBasicSplitPlanningDeleteFiles() {


I don't see a reason why we wouldn't store that. I think it was overlooked.
@szehon-ho, can we do that in a follow-up PR? Not urgent, at least we should create an issue.

aokolnychyi · 2023-01-24T21:55:11Z

Thanks, @szehon-ho! Thanks for reviewing, @RussellSpitzer!

szehon-ho · 2023-01-24T21:59:07Z

Thanks, created #6658 and #6659 to track existing issues found here.

…6365)

github-actions Bot added the core label Dec 6, 2022

szehon-ho force-pushed the position_delete_table_only branch from f1ff895 to a99a078 Compare December 6, 2022 04:54

szehon-ho commented Dec 7, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/SerializableTable.java Outdated

szehon-ho commented Dec 8, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

RussellSpitzer reviewed Dec 15, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/AbstractTableScan.java Outdated

RussellSpitzer reviewed Dec 15, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/AbstractTableScan.java Outdated

szehon-ho commented Dec 15, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/AbstractTableScan.java

RussellSpitzer reviewed Dec 15, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetadataTable.java Outdated

RussellSpitzer reviewed Dec 19, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseTableScan.java Outdated

RussellSpitzer reviewed Dec 19, 2022

View reviewed changes

RussellSpitzer reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

RussellSpitzer reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

RussellSpitzer reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java

RussellSpitzer reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/test/java/org/apache/iceberg/TestMetadataTableScansWithPartitionEvolution.java

RussellSpitzer reviewed Dec 20, 2022

View reviewed changes

aokolnychyi reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Dec 20, 2022

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

szehon-ho force-pushed the position_delete_table_only branch from f2b5ff1 to fb6faab Compare December 21, 2022 18:47

szehon-ho commented Dec 21, 2022

View reviewed changes

aokolnychyi reviewed Dec 22, 2022

View reviewed changes

szehon-ho commented Jan 5, 2023

View reviewed changes

szehon-ho force-pushed the position_delete_table_only branch from aeabee3 to e053f26 Compare January 10, 2023 01:31

aokolnychyi reviewed Jan 17, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Jan 17, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Jan 17, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

aokolnychyi reviewed Jan 18, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java

aokolnychyi reviewed Jan 18, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java Outdated

szehon-ho force-pushed the position_delete_table_only branch from 62e620f to e2c03aa Compare January 18, 2023 02:19

github-actions Bot added the API label Jan 19, 2023

szehon-ho force-pushed the position_delete_table_only branch from 3623d15 to c326236 Compare January 19, 2023 00:58

szehon-ho mentioned this pull request Jan 19, 2023

Partitions metadata table shows old partitions #6257

Closed

szehon-ho force-pushed the position_delete_table_only branch 2 times, most recently from b84e94c to 93d07ef Compare January 20, 2023 08:16

aokolnychyi reviewed Jan 20, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/PositionDeletesTable.java

aokolnychyi reviewed Jan 20, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/SnapshotScan.java Outdated

aokolnychyi reviewed Jan 20, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/iceberg/BaseMetadataTable.java

szehon-ho force-pushed the position_delete_table_only branch from 29bd430 to b4e641a Compare January 20, 2023 22:58

szehon-ho commented Jan 21, 2023

View reviewed changes

aokolnychyi approved these changes Jan 24, 2023

View reviewed changes

Add position delete table

767474a

szehon-ho force-pushed the position_delete_table_only branch from ebf59ba to 767474a Compare January 24, 2023 19:13

aokolnychyi merged commit 3c27c57 into apache:master Jan 24, 2023

nastra mentioned this pull request Jan 25, 2023

Core: Fix API breakages around scanMetrics() #6664

Merged

This was referenced Feb 1, 2023

Spark 3.3: Implement Position Deletes Table #6716

Merged

Spark 3.3: Add RemoveDanglingDeletes action #6581

Closed

Core: Support delete file stats in partitions metadata table #6661

Merged

szehon-ho mentioned this pull request Mar 16, 2023

iceberg v2 table cannot expire delete files after rewrite datafile action #5058

Closed

krvikash pushed a commit to krvikash/iceberg that referenced this pull request Mar 16, 2023

API, Core: Add position deletes metadata table (apache#6365)

4870047

zhongyujiang pushed a commit to zhongyujiang/iceberg that referenced this pull request Apr 16, 2025

[Cherry-Pick] API, Core: Add position deletes metadata table (apache#…

e60876e

…6365)


		List<ScanTask> tasks = Lists.newArrayList(scan.planFiles());

		Assert.assertEquals(

Conversation

szehon-ho commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RussellSpitzer left a comment

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Dec 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szehon-ho commented Dec 6, 2022 •

edited

Loading

szehon-ho Dec 21, 2022 •

edited

Loading

szehon-ho Jan 5, 2023 •

edited

Loading

szehon-ho Jan 5, 2023 •

edited

Loading