Core: Add delete marker metadata column #2538

chenjunjiedada · 2021-04-28T12:24:20Z

This adds a metadata column to indicate whether a row is deleted or not. A delete marker column can be used when finding the deleted rows as we discussed in #2372, it can also be used to simplify the overall merge on read process. In order to avoid overhead, the delete marker metadata column only projected when the delete files exist.

chenjunjiedada · 2021-04-28T12:26:24Z

This is a separate part from #2372. cc @openinx @rdblue @aokolnychyi @RussellSpitzer @yyanyy

RussellSpitzer · 2021-04-28T12:48:27Z

To be clear, this PR is to enable reading of data while optionally marking rows as deleted rather than actually filtering them out so the delete information can be used in other utilities to rewrite delete files into more compact forms?

chenjunjiedada · 2021-04-28T13:20:04Z

@RussellSpitzer you are right! With a delete marker column, we can also simplify the current filtering logic a bit.

data/src/main/java/org/apache/iceberg/data/DeleteFilter.java

yyanyy · 2021-04-30T00:58:09Z

flink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java


  public static void assertTableRecords(Table table, List<Record> expected) throws IOException {
    table.refresh();
+


Why do we need this change? is it because two sets don't match after we add the additional metadata column? but wouldn't _pos and _file also have this issue?

Because the metadata column projection logic produces additional columns even when the requestedSchema doesn't contain them and that is why we use StructLikeSet. The _pos column shows up only when positional deletes exist, the _deleted marker shows up when any of the deletes exist.

The failed unit test contains only equality delete which produces only _deleted column, so it failed with HashMultiSet comparison. But when the unit test, for example TestIcebergFilesCommitter.TestCommitTwoCheckpointsInSingleTxn, contains a positional delete, the unit test fails as well due to it has the additional column _pos. The following patch for the unit test could test it.

- DeleteFile deleteFile1 = writeEqDeleteFile(appenderFactory, "delete-file-1", ImmutableList.of(delete3)); + DeleteFile deleteFile1 = writePosDeleteFile(appenderFactory, + "pos-delete-file-1", + ImmutableList.of(Pair.of(dataFile1.path(), 3L)));

I also find this suspicious. Is the extra column in the expected records or the table? I don't think that this PR should change the data produced by IcebergGenerics.read(table).build().

It is in expected records. The extra column is added in DeleteFileter#fileProjection.

private static Schema fileProjection(Schema tableSchema, Schema requestedSchema, List<DeleteFile> posDeletes, List<DeleteFile> eqDeletes) { ... // We add it to requiredIds, so that it exists in missingIds when requestedSchema doesn't contain it. if (!posDeletes.isEmpty()) { requiredIds.add(MetadataColumns.ROW_POSITION.fieldId()); } .... // We append it at the end anyway. if (missingIds.contains(MetadataColumns.ROW_POSITION.fieldId())) { columns.add(MetadataColumns.ROW_POSITION); } return new Schema(columns); }

chenjunjiedada · 2021-05-07T03:27:13Z

cc @jackye1995 as well.

I added a couple of unit tests for this, more unit tests could be added in the deletes reader PR. @RussellSpitzer @yyanyy @rdblue @openinx, could you please help to take a look when you have time?

jackye1995 · 2021-05-20T22:21:29Z

overall looks good to me, I left some discussion comments in the original PR.

chenjunjiedada · 2021-05-21T12:43:59Z

Thanks @jackye1995 !

rdblue · 2021-05-21T22:16:39Z

core/src/main/java/org/apache/iceberg/MetadataColumns.java

      Integer.MAX_VALUE - 1, "_file", Types.StringType.get(), "Path of the file in which a row is stored");
  public static final NestedField ROW_POSITION = NestedField.required(
      Integer.MAX_VALUE - 2, "_pos", Types.LongType.get(), "Ordinal position of a row in the source data file");
+  public static final NestedField DELETE_MARK = NestedField.required(


Instead of DELETE_MARK, how about IS_DELETED? I don't think that "mark" is clear enough to describe what this is. Similarly, I think the docs should be "Whether the row has been deleted". There's no need to include "delete mark" because that's identifying something that is not defined (this column is _deleted and "mark" is not introduced), and "or not" is unnecessary because it is implied by "whether".

rdblue · 2021-05-21T22:18:06Z

data/src/test/java/org/apache/iceberg/data/DeleteReadTests.java

  }

-  protected StructLikeSet rowSetWitIds(int... idsToRetain) {
+  protected StructLikeSet rowSetWithIds(int... idsToRetain) {


Can you fix this in a separate PR? This file doesn't need to change and it could cause commit conflicts.

rdblue · 2021-05-21T22:19:26Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueReaders.java

    }
  }

+  static class DeleteMarkerReader implements ParquetValueReader<Boolean> {


Isn't there a constant reader that we can reuse?

Yes, we could use the existing constant reader.

rdblue · 2021-05-21T22:20:06Z

spark/src/main/java/org/apache/iceberg/spark/data/vectorized/RowPositionColumnVector.java

  private final long batchOffsetInFile;

-  RowPostitionColumnVector(long batchOffsetInFile) {
+  RowPositionColumnVector(long batchOffsetInFile) {


Can we fix typos in a separate PR? This is already touching quite a few files and this file doesn't need to change.

rdblue · 2021-05-21T22:20:56Z

orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java

    }
  }
+
+  private static class DeleteMarkReader implements OrcValueReader<Boolean> {


Is there a constant reader that could be reused?

We could use the existing constant reader from parquet and orc readers. I also created a new constant reader for Avro reader.

chenjunjiedada · 2021-05-26T02:41:38Z

Thanks @rdblue for reviewing! I addressed your comments.

chenjunjiedada · 2021-06-15T01:26:27Z

@rdblue Is this ready to merge? The delete rewrites may depend on this.

rdblue · 2021-06-15T18:16:04Z

Thanks, @chenjunjiedada. I'll take a look.

rdblue · 2021-06-15T20:31:27Z

core/src/main/java/org/apache/iceberg/avro/ValueReaders.java

          // track where the _pos field is located for setRowPositionSupplier
          this.posField = pos;
+        } else if (field.fieldId() == MetadataColumns.IS_DELETED.fieldId()) {
+          this.readers[pos] = new ConstantReader<>(false);


Constants are already handled by this class, see the first branch of this if/else logic. I think that it would make more sense to reuse that rather than create a new constant reader.

rdblue · 2021-06-15T20:37:42Z

@chenjunjiedada, the constant handling in Avro appears to be the only remaining issue. Once you update that, I'll merge this. Thanks for working on it!

rdblue · 2021-06-16T00:56:41Z

core/src/main/java/org/apache/iceberg/avro/ValueReaders.java

    }
  }
+
+  static class ConstantReader<C> implements ValueReader<C> {


@chenjunjiedada, since this should no longer be needed, can you please remove it?

Sure，I should remove this first to track all its usage. I believe now no one is using it.

rdblue · 2021-06-16T00:57:36Z

core/src/main/java/org/apache/iceberg/avro/ValueReaders.java

          // track where the _pos field is located for setRowPositionSupplier
          this.posField = pos;
+        } else if (AvroSchemaUtil.getFieldId(field) == MetadataColumns.IS_DELETED.fieldId()) {
+          this.readers[pos] = new ConstantReader<>(false);


Can you convert this to use positions and constants as well?

Oops, my brain was not working well this early morning... Let me take a coffee first.

chenjunjiedada · 2021-06-17T23:41:00Z

@rdblue , The last comment was addressed, could we merge this?

rdblue · 2021-06-18T23:38:11Z

Thanks for pinging me, @chenjunjiedada! This looks good now, I'll merge it.

chenjunjiedada · 2021-06-18T23:48:34Z

Thanks for the reviewing and merging! @rdblue @jackye1995 @yyanyy !

github-actions bot added core data flink ORC parquet spark labels Apr 28, 2021

github-actions bot added the arrow label Apr 28, 2021

fix ut

880fad1

yyanyy reviewed Apr 30, 2021

View reviewed changes

chenjunjiedada marked this pull request as ready for review May 7, 2021 01:49

chenjunjiedada added 4 commits May 7, 2021 09:51

Merge branch 'master' of https://github.com/apache/iceberg

078f0d3

Core: Add delete marker metadata column

cb9a640

Add unit test

2029b32

use constant reader

3e284b2

chenjunjiedada force-pushed the add-delete-marker-metacolumn branch from 83356a5 to 3e284b2 Compare May 7, 2021 02:32

rdblue reviewed May 21, 2021

View reviewed changes

chenjunjiedada force-pushed the add-delete-marker-metacolumn branch from ef12b5f to 4f76b4a Compare May 22, 2021 02:30

use constant reader

bf7d7ad

chenjunjiedada force-pushed the add-delete-marker-metacolumn branch from 4f76b4a to bf7d7ad Compare May 22, 2021 02:57

Merge branch 'master' into add-delete-marker-metacolumn

ed0b04d

rdblue reviewed Jun 15, 2021

View reviewed changes

use existing constant reader

f290240

rdblue reviewed Jun 16, 2021

View reviewed changes

use existing constant reader

3380c7d

rdblue approved these changes Jun 18, 2021

View reviewed changes

rdblue merged commit a9f4363 into apache:master Jun 18, 2021

flyrain mentioned this pull request May 2, 2022

Read deleted rows with metadata column IS_DELETED #4683

Merged


		public static void assertTableRecords(Table table, List<Record> expected) throws IOException {
		table.refresh();

Core: Add delete marker metadata column #2538

Core: Add delete marker metadata column #2538

Uh oh!

Conversation

chenjunjiedada commented Apr 28, 2021

Uh oh!

chenjunjiedada commented Apr 28, 2021

Uh oh!

RussellSpitzer commented Apr 28, 2021

Uh oh!

chenjunjiedada commented Apr 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenjunjiedada Apr 30, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chenjunjiedada commented May 7, 2021

Uh oh!

jackye1995 commented May 20, 2021

Uh oh!

chenjunjiedada commented May 21, 2021

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!

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!

chenjunjiedada commented May 26, 2021

Uh oh!

chenjunjiedada commented Jun 15, 2021

Uh oh!

rdblue commented Jun 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Jun 15, 2021

Uh oh!

rdblue Jun 16, 2021 • 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!

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!

chenjunjiedada commented Jun 17, 2021

Uh oh!

rdblue commented Jun 18, 2021

Uh oh!

chenjunjiedada commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

chenjunjiedada Apr 30, 2021 •

edited

Loading

rdblue Jun 16, 2021 •

edited

Loading