Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.apache.iceberg.spark.extensions;

import static org.junit.Assert.assertThrows;

import java.util.List;
import java.util.Map;
import org.apache.iceberg.ChangelogOperation;
Expand Down Expand Up @@ -411,6 +413,54 @@ public void testRemoveCarryOversWithoutUpdatedRows() {
sql("select * from %s order by _change_ordinal, id, data", viewName));
}

@Test
public void testNetChangesWithRemoveCarryOvers() {
// partitioned by id
createTableWith3Columns();
Comment thread
flyrain marked this conversation as resolved.
Outdated

// insert rows: (1, 'a', 12) (2, 'b', 11) (2, 'e', 12)
sql("INSERT INTO %s VALUES (1, 'a', 12), (2, 'b', 11), (2, 'e', 12)", tableName);
Table table = validationCatalog.loadTable(tableIdent);
Snapshot snap1 = table.currentSnapshot();

// delete rows: (2, 'b', 11) (2, 'e', 12), insert rows: (3, 'c', 13) (2, 'd', 11) (2, 'e', 12)
Comment thread
flyrain marked this conversation as resolved.
Outdated
sql("INSERT OVERWRITE %s VALUES (3, 'c', 13), (2, 'd', 11), (2, 'e', 12)", tableName);
table.refresh();
Snapshot snap2 = table.currentSnapshot();

// delete rows: (2, 'd', 11) (2, 'e', 12) (3, 'c', 13), insert rows: (3, 'c', 15) (2, 'e', 12)
sql("INSERT OVERWRITE %s VALUES (3, 'c', 15), (2, 'e', 12)", tableName);
table.refresh();
Snapshot snap3 = table.currentSnapshot();

List<Object[]> returns =
sql(
"CALL %s.system.create_changelog_view(table => '%s', net_changes => true)",
catalogName, tableName);

String viewName = (String) returns.get(0)[0];

assertEquals(
"Rows should match",
ImmutableList.of(
row(1, "a", 12, INSERT, 0, snap1.snapshotId()),
row(3, "c", 15, INSERT, 2, snap3.snapshotId()),
row(2, "e", 12, INSERT, 2, snap3.snapshotId())),
sql("select * from %s order by _change_ordinal, data", viewName));
}

@Test
public void testNetChangesWithComputeUpdates() {
Comment thread
flyrain marked this conversation as resolved.
createTableWith2Columns();
assertThrows(
"Should fail because net_changes is not supported with computing updates",
IllegalArgumentException.class,
() ->
sql(
"CALL %s.system.create_changelog_view(table => '%s', identifier_columns => array('id'), net_changes => true)",
catalogName, tableName));
}

@Test
public void testNotRemoveCarryOvers() {
createTableWith3Columns();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ public static Iterator<Row> removeCarryovers(Iterator<Row> rowIterator, StructTy
return Iterators.filter(changelogIterator, Objects::nonNull);
}

public static Iterator<Row> removeNetCarryovers(Iterator<Row> rowIterator, StructType rowType) {
Comment thread
flyrain marked this conversation as resolved.
ChangelogIterator changelogIterator = new RemoveNetCarryoverIterator(rowIterator, rowType);
return Iterators.filter(changelogIterator, Objects::nonNull);
}

protected boolean isDifferentValue(Row currentRow, Row nextRow, int idx) {
return !Objects.equals(nextRow.get(idx), currentRow.get(idx));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,16 @@
*/
class RemoveCarryoverIterator extends ChangelogIterator {
private final int[] indicesToIdentifySameRow;
private final StructType rowType;

private Row cachedDeletedRow = null;
private long deletedRowCount = 0;
private Row cachedNextRecord = null;

RemoveCarryoverIterator(Iterator<Row> rowIterator, StructType rowType) {
super(rowIterator, rowType);
this.indicesToIdentifySameRow = generateIndicesToIdentifySameRow(rowType.size());
this.rowType = rowType;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if we pass the variable to the super class , can we just store it in super class and get it via super method? (I think cleaner if subclass has only fields that it only knows about).

Also looks like both subclass do this.

this.indicesToIdentifySameRow = generateIndicesToIdentifySameRow();
}

@Override
Expand Down Expand Up @@ -139,8 +141,8 @@ private boolean hasCachedDeleteRow() {
return cachedDeletedRow != null;
}

private int[] generateIndicesToIdentifySameRow(int columnSize) {
int[] indices = new int[columnSize - 1];
private int[] generateIndicesToIdentifySameRow() {
int[] indices = new int[rowType.size() - 1];
for (int i = 0; i < indices.length; i++) {
if (i < changeTypeIndex()) {
indices[i] = i;
Expand All @@ -151,13 +153,21 @@ private int[] generateIndicesToIdentifySameRow(int columnSize) {
return indices;
}

private boolean isSameRecord(Row currentRow, Row nextRow) {
for (int idx : indicesToIdentifySameRow) {
protected boolean isSameRecord(Row currentRow, Row nextRow) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to base class?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class ComputeUpdateIterator doesn't need it. Can we do that until all subclasses of ChangelogIterator need it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise, I am thinking that it is a bit harder to read to have RemoveNetCarryoverIterator to extend RemoveCarryoverIterator. So was thinking we can move have an extra base class, then its easier to see what is the different methods and what is same, not sure what you think. Now its a bit trickier to see that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to ChangelogIterator, and let RemoveNetCarryoverIterator inherit ChangelogIterator directly. It should be easier to read.

for (int idx : indicesToIdentifySameRow()) {
Comment thread
flyrain marked this conversation as resolved.
Outdated
if (isDifferentValue(currentRow, nextRow, idx)) {
return false;
}
}

return true;
}

protected StructType rowType() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move these up to base class ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class ComputeUpdateIterator doesn't need it. Can we do that until all subclasses of ChangelogIterator need it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

return rowType;
}

protected int[] indicesToIdentifySameRow() {
return indicesToIdentifySameRow;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/*
* 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.spark;

import java.util.Iterator;
import java.util.List;
import org.apache.iceberg.MetadataColumns;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.spark.sql.Row;
import org.apache.spark.sql.types.StructType;

/**
* This class computes the net changes across multiple snapshots. It takes a row iterator, and
Comment thread
flyrain marked this conversation as resolved.
Outdated
* assumes the following:
*
* <ul>
* <li>The row iterator is partitioned by all columns.
* <li>The row iterator is sorted by all columns, change order, and change type. The change order
* is 1-to-1 mapping to snapshot id.
* </ul>
*/
public class RemoveNetCarryoverIterator extends RemoveCarryoverIterator {

@flyrain flyrain May 19, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've got this new iterator that can do everything RemoveCarryoverIterator can and more. I'm thinking we could merge them. It'd use more memory since it caches rows. If the memory increase isn't an issue, this could simplify our code. Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to use the list here? Don't we only need to know the first row and the last?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look a bit more. We should be able to archive the same without a list. Let me post a new commit soon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the list since we can either cache a delete row or a insert row. But looking a bit more. It is a either-or at any time. We will never have cached rows with mixed insert/delete, since we will remove them as a pair in that case. For example, cached rows could be

(1, 'a', delete)
(1, 'a', delete)
(1, 'a', delete)

or

(1, 'a', insert)
(1, 'a', insert)

But it will never be

(1, 'a', delete)
(1, 'a', insert)

With that, a cached row count is good enough. Removed the list in the new commit. This new iterator can do more than its parent with the same cost. I think it is good idea to just keep one. Thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one iterator is fine

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me merge them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code becomes entangled when I merge them. Too many if-else clauses. We'd better off leave as it is.


private final List<Row> cachedRows = Lists.newArrayList();
private final int[] indicesToIdentifySameRow;

private Row cachedNextRow = null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rely on java default


protected RemoveNetCarryoverIterator(Iterator<Row> rowIterator, StructType rowType) {
super(rowIterator, rowType);
this.indicesToIdentifySameRow = generateIndicesToIdentifySameRow();
}

@Override
public boolean hasNext() {
if (!cachedRows.isEmpty()) {
return true;
}

if (cachedNextRow != null) {
return true;
}

return rowIterator().hasNext();
}

@Override
public Row next() {
// if there are cached rows, return one of them from the beginning
if (!cachedRows.isEmpty()) {
return cachedRows.remove(0);
}

Row currentRow = getCurrentRow();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for 'cachedRow'. Can we just have currentRow as member variable, and do

this.currentRow = getCurrentRow();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get it. Could you elaborate it?


// if there is no next row, return the current row
Comment thread
flyrain marked this conversation as resolved.
Outdated
if (!rowIterator().hasNext()) {
return currentRow;
}

Row nextRow = rowIterator().next();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure, but instead of 'cachedNextRow', can we just have 'nextRow' as the member variable.

Then 'this.nextRow = rowIterator.next()'?

I think 'cachedNextRow' is used only in getCurrentRow() which is above this code. So henceforth should be the same?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When cachedRowCount is larger than zero, we have to drain it first, then get the cached next row if there is one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(1, 'a', insert)
(1, 'a', insert)
(1, 'b', insert)
(1, 'b', delete)

Here is an example, we need to drain 'a' first, then get the cached next row (1, 'b', insert), and try to match it with the row (1, 'b', delete)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not clear:


  @Override
  public Row next() {
    // if there are cached rows, return one of them from the beginning
    if (cachedRowCount > 0) {
      cachedRowCount--;
      return cachedRow;
    }

    this.cachedRow = getCurrentRow();

    // return it directly if the current row is the last row
    if (!rowIterator().hasNext()) {
      return cachedRow;
    }

    this.cachedNextRow = rowIterator().next();
    cachedRowCount = 1;

    // pull rows from the iterator until two consecutive rows are different
    while (isSameRecord(cachedRow, cachedNextRow)) {
      if (oppositeChangeType(cachedRow, cachedNextRow)) {
        // two rows with opposite change types means no net changes, remove both
        cachedRowCount--;
      } else {
        // two rows with same change types means potential net changes, cache the next row, reset it
        // to null
        cachedRowCount++;
      }

      // stop pulling rows if there is no more rows or the next row is different
      if (cachedRowCount <= 0 || !rowIterator().hasNext()) {
        this.cachedNextRow = null;
        break;
      }

      this.cachedNextRow = rowIterator().next();
    }

    return null;
  }

I think it will work to remove 'currentRow'. But I am not sure if I am too aggressive for removing 'nextRow'. Please check if I made a mistake.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change accordingly. Thanks for the suggestion


cachedRows.add(currentRow);
// if they are the same row, remove the cached row or stack to the cached row
Comment thread
flyrain marked this conversation as resolved.
Outdated
while (isSameRecord(currentRow, nextRow)) {
if (matched(currentRow, nextRow)) {
// if matched, remove both rows, remove the last row in the cache
cachedRows.remove(cachedRows.size() - 1);
nextRow = null;
} else {
// stack the row into the cache
cachedRows.add(nextRow);
nextRow = null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe missing something, but why do we need to nullify nextRow here? And in both if/else cases?

@flyrain flyrain Jun 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cached the next row by doing cachedRowCount++ since they are the same record with different change types, we need to reset it nextRow to be null. Otherwise, it will come out of the loop from here:

      if (cachedRowCount <= 0 || !rowIterator().hasNext()) {
        break;
      }

Then caught by

    // if they are different rows, hit the boundary, cache the next row
    cachedNextRow = nextRow;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep both nextRow=null in if-clause and else-clause since they serve different purposes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, let's move it out of the if/else. Even Intellij suggests 'Common part can be extracted from 'if'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still keep them there. I will consider a plus for people to understand the code, though there is a bit duplication.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consolidated them to one line.

}

// break the loop if there is no next row or the cache is empty
Comment thread
flyrain marked this conversation as resolved.
Outdated
if (cachedRows.isEmpty() || !rowIterator().hasNext()) {
break;
}

// get the current row from the cache, and get the next row from the iterator for the next
// loop
currentRow = cachedRows.get(cachedRows.size() - 1);
nextRow = rowIterator().next();
}

// if they are different rows, hit the boundary, cache the next row
cachedNextRow = nextRow;
return null;
}

private Row getCurrentRow() {
Row currentRow;
if (cachedNextRow != null) {
currentRow = cachedNextRow;
cachedNextRow = null;
} else {
currentRow = rowIterator().next();
}
return currentRow;
}

private boolean matched(Row currentRow, Row nextRow) {
Comment thread
flyrain marked this conversation as resolved.
Outdated
return (nextRow.getString(changeTypeIndex()).equals(INSERT)
Comment thread
flyrain marked this conversation as resolved.
Outdated
&& currentRow.getString(changeTypeIndex()).equals(DELETE))
|| (nextRow.getString(changeTypeIndex()).equals(DELETE)
&& currentRow.getString(changeTypeIndex()).equals(INSERT));
}

private int[] generateIndicesToIdentifySameRow() {
int changeOrdinalIndex = rowType().fieldIndex(MetadataColumns.CHANGE_ORDINAL.name());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit lost why this method is not the same as RemoveCarryoverIterator? Are 'changeOrdinalIndex' and 'snapshotIdIndex' not in the rows of the other iterator?

@flyrain flyrain Jun 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indices generated here is used for comparing if two records are the same. In RemoveCarryoverIterator, we consider two records are different if their changeOrdinalIndex and/or snapshotIdIndex are not the same, while we may consider two records are the same in RemoveNetCarryoverIterator even if these two columns are different. We will need the snapshot boundary in RemoveCarryoverIterator since it is only for one single snapshot. For example, we cannot merge the following two rows in RemoveCarryoverIterator, while we can in RemoveNetCarryoverIterator

(1, 'a', insert, 'snapshot-1')
(1, 'a', delete, 'snapshot-2')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant we share the code by doing the same thing and making a set to identify metadata column index?

  private int[] generateIndicesToIdentifySameRow() {
    Set<Integer> metadataColumnIndices = Sets.newHashSet(
        rowType().fieldIndex(MetadataColumns.CHANGE_ORDINAL.name()),
        rowType().fieldIndex(MetadataColumns.COMMIT_SNAPSHOT_ID.name()),
        changeTypeIndex());
    return generateIndicesToIdentifySameRow(metadataColumnIndices);
  }

  private int[] generateIndicesToIdentifySameRow(Set<Integer> metadataColumnIndices) {
    int[] indices = new int[rowType().size() - metadataColumnIndices.size()];

    for (int i = 0, j = 0; i < indices.length; i++) {
      if (!metadataColumnIndices.contains(i)) {
        indices[j] = i;
        j++;
      }
    }
    return indices;
  }

From RemoveCarryoverIterator, the set will be only changeTypeIndex? Let me know if I miss something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract the method out. Thanks for the suggestion.

int snapshotIdIndex = rowType().fieldIndex(MetadataColumns.COMMIT_SNAPSHOT_ID.name());

int[] indices = new int[rowType().size() - 3];

for (int i = 0, j = 0; i < indices.length; i++) {
if (i != changeTypeIndex() && i != changeOrdinalIndex && i != snapshotIdIndex) {
indices[j] = i;
j++;
}
}
return indices;
}

@Override
protected int[] indicesToIdentifySameRow() {
return indicesToIdentifySameRow;
}
}
Loading