Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
10 changes: 10 additions & 0 deletions api/src/main/java/org/apache/iceberg/TableScan.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ public interface TableScan extends Scan<TableScan, FileScanTask, CombinedScanTas
*/
TableScan asOfTime(long timestampMillis);

/**
* Create a new {@link TableScan} from this scan's configuration that will use the most recent
* snapshot as of the given snapshot ref.
*
* @param snapshotRef a snapshot Ref
* @return a new scan based on this with the given snapshot Ref
* @throws IllegalArgumentException if the snapshot cannot be found
*/
TableScan useSnapshotRef(String snapshotRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this this should be useRef(String branchOrTagName). The term SnapshotRef is internal and I don't think it should be exposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to separate the useBranch and useTag APIs. As you said, refs are internal. From a Spark user perspective we also want to only expose the branch/tag terms; imo I think the same case could be applied to the API level. Also considering branches can be combined with time travel we could do a separate API for that ; although there's an argument to be made to just combine useBranch + as Of Time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I considered that as well. The problem is that the caller doesn't know whether the ref is a tag or a branch before calling the method. That's determined when we look at table metadata and we don't want to force the caller to do that.

There may be a better name than "ref" for useRef. That seems like the problem to me. Maybe we could simplify it to use? I'm not sure that's obvious enough.

@aokolnychyi, do you have any thoughts on the name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rdblue @amogh-jahagirdar I agree that we can use a common API for tag or branch like useRef.

We have two signatures:

useRef(String refName)

useRef(String refName, Long timeStampMillis) -> will throw exception for tag type, since we cant do time travel for tag.

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Aug 9, 2022

Choose a reason for hiding this comment

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

Sure, this sounds reasonable. The only thing is I think if we do useRef (or if we come up with a better name) then we would not want to have the useRef(String refName, Long timeStampMillis). A user would chain it with the existing useTimestamp and then the validation that it's a branch would happen in the scan context.useRef().asOfTime() I don't think we would want the extra method because time travel would only apply for branches so having the ref in that case doesn't make sense to me since it's really only supported for 1 ref type, the branch.

If we have consensus on this, then I can update https://github.com/apache/iceberg/pull/5364/files with the updated approach. Then this, PR could be focused on the Spark side of the integration. Will wait to hear what @aokolnychyi suggests as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

will throw exception for tag type, since we cant do time travel for tag.

In that case I would suggest:

  • useRef(String refName)
  • useBranchAsOfTime(String branchName, Long timeStampMillis)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see the alternative is just .useRef(refName).asOfTime(timestampMillis). That also works, in that case +1 for useRef(String refName)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like there is consensus for useRef.


/**
* Create a new {@link TableScan} from this that will read the given data columns. This produces
* an expected schema that includes all fields that are either selected or used by this scan's
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/org/apache/iceberg/BaseTableScan.java
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,13 @@ public TableScan useSnapshot(long scanSnapshotId) {
tableOps(), table(), tableSchema(), context().useSnapshotId(scanSnapshotId));
}

@Override
public TableScan useSnapshotRef(String snapshotRef) {
Preconditions.checkArgument(
table().snapshot(snapshotRef) != null, "Cannot find ref with name %s", snapshotRef);
return useSnapshot(table().snapshot(snapshotRef).snapshotId());
}

@Override
public TableScan asOfTime(long timestampMillis) {
Preconditions.checkArgument(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,14 @@ public Long endSnapshotId() {
return confParser.longConf().option(SparkReadOptions.END_SNAPSHOT_ID).parseOptional();
}

public String branch() {
return confParser.stringConf().option(SparkReadOptions.BRANCH).parseOptional();
}

public String tag() {
return confParser.stringConf().option(SparkReadOptions.TAG).parseOptional();
}

public String fileScanTaskSetId() {
return confParser.stringConf().option(SparkReadOptions.FILE_SCAN_TASK_SET_ID).parseOptional();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ private SparkReadOptions() {}
// A timestamp in milliseconds; the snapshot used will be the snapshot current at this time.
public static final String AS_OF_TIMESTAMP = "as-of-timestamp";

// branch ref of the table snapshot to read from
public static final String BRANCH = "branch";

// tag ref of the table snapshot to read from
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit: I think for the comments we can leave off the "table snapshot" and "ref" part and the comment could look something like

"Tag to read from"
"Branch to read from"

public static final String TAG = "tag";

// Overrides the table's read.split.target-size and read.split.metadata-target-size
public static final String SPLIT_SIZE = "split-size";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class SparkBatchQueryScan extends SparkScan implements SupportsRuntimeFiltering
private final Long startSnapshotId;
private final Long endSnapshotId;
private final Long asOfTimestamp;
private final String branch;
private final String tag;
private final List<Expression> runtimeFilterExpressions;

private Set<Integer> specIds = null; // lazy cache of scanned spec IDs
Expand All @@ -88,6 +90,8 @@ class SparkBatchQueryScan extends SparkScan implements SupportsRuntimeFiltering
this.startSnapshotId = readConf.startSnapshotId();
this.endSnapshotId = readConf.endSnapshotId();
this.asOfTimestamp = readConf.asOfTimestamp();
this.branch = readConf.branch();
this.tag = readConf.tag();
this.runtimeFilterExpressions = Lists.newArrayList();

if (scan == null) {
Expand Down Expand Up @@ -244,6 +248,13 @@ public Statistics estimateStatistics() {
Snapshot snapshot = table().snapshot(snapshotIdAsOfTime);
return estimateStatistics(snapshot);

} else if (branch != null) {
Snapshot snapshot = table().snapshot(branch);
return estimateStatistics(snapshot);

} else if (tag != null) {
Snapshot snapshot = table().snapshot(tag);
return estimateStatistics(snapshot);
} else {
Snapshot snapshot = table().currentSnapshot();
return estimateStatistics(snapshot);
Expand All @@ -269,7 +280,9 @@ && readSchema().equals(that.readSchema())
&& Objects.equals(snapshotId, that.snapshotId)
&& Objects.equals(startSnapshotId, that.startSnapshotId)
&& Objects.equals(endSnapshotId, that.endSnapshotId)
&& Objects.equals(asOfTimestamp, that.asOfTimestamp);
&& Objects.equals(asOfTimestamp, that.asOfTimestamp)
&& Objects.equals(branch, that.branch)
&& Objects.equals(branch, that.tag);
}
Copy link
Contributor Author

@namrathamyske namrathamyske Oct 30, 2022

Choose a reason for hiding this comment

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

have to uncomment this , but getting a checkstyle cyclomatic complexity error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering it's required for a correct equals implementation of SparkBatchQueryScan, I think it makes the most sense just to suppress the warnings on the method @SuppressWarnings("checkstyle:CyclomaticComplexity")


@Override
Expand All @@ -282,7 +295,9 @@ public int hashCode() {
snapshotId,
startSnapshotId,
endSnapshotId,
asOfTimestamp);
asOfTimestamp,
branch,
tag);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.arrow.util.Preconditions;
import org.apache.iceberg.MetadataColumns;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Snapshot;
Expand Down Expand Up @@ -182,13 +183,28 @@ private Schema schemaWithMetadataColumns() {
public Scan build() {
Long snapshotId = readConf.snapshotId();
Long asOfTimestamp = readConf.asOfTimestamp();
String branch = readConf.branch();
String tag = readConf.branch();

Preconditions.checkArgument(
branch == null || tag == null,
"Cannot set both %s and %s to select which table snapshot to scan",
SparkReadOptions.BRANCH,
SparkReadOptions.TAG);

Preconditions.checkArgument(
snapshotId == null || asOfTimestamp == null,
"Cannot set both %s and %s to select which table snapshot to scan",
SparkReadOptions.SNAPSHOT_ID,
SparkReadOptions.AS_OF_TIMESTAMP);

String snapshotRef = branch != null ? branch : tag;
Preconditions.checkArgument(
snapshotId == null || snapshotRef == null,
"Cannot set both %s and %s to select which table snapshot to scan",
SparkReadOptions.SNAPSHOT_ID,
"branch/tag");

Long startSnapshotId = readConf.startSnapshotId();
Long endSnapshotId = readConf.endSnapshotId();

Expand Down Expand Up @@ -225,6 +241,12 @@ public Scan build() {
scan = scan.asOfTime(asOfTimestamp);
}

if (branch != null) {
scan.useSnapshotRef(branch);
} else if (tag != null) {
scan.useSnapshotRef(tag);
}

if (startSnapshotId != null) {
if (endSnapshotId != null) {
scan = scan.appendsBetween(startSnapshotId, endSnapshotId);
Expand All @@ -240,7 +262,10 @@ public Scan build() {

public Scan buildMergeOnReadScan() {
Preconditions.checkArgument(
readConf.snapshotId() == null && readConf.asOfTimestamp() == null,
readConf.snapshotId() == null
&& readConf.asOfTimestamp() == null
&& readConf.branch() == null
&& readConf.tag() == null,
"Cannot set time travel options %s and %s for row-level command scans",
SparkReadOptions.SNAPSHOT_ID,
SparkReadOptions.AS_OF_TIMESTAMP);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,88 @@ public void testSnapshotSelectionBySnapshotIdAndTimestamp() throws IOException {
.hasMessageContaining("Cannot specify both snapshot-id")
.hasMessageContaining("and as-of-timestamp");
}

@Test
public void testSnapshotSelectionByTag() throws IOException {
Copy link
Contributor

@rdblue rdblue Oct 17, 2022

Choose a reason for hiding this comment

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

I think we also need tests to show that branch and tag options can't be used at the same time, and tests to validate what happens when snapshot or timestamp are set along with branch or tag. It should be easy to make a few tests for those error cases.

String tableLocation = temp.newFolder("iceberg-table").toString();

HadoopTables tables = new HadoopTables(CONF);
PartitionSpec spec = PartitionSpec.unpartitioned();
Table table = tables.create(SCHEMA, spec, tableLocation);

// produce the first snapshot
List<SimpleRecord> firstBatchRecords = Lists.newArrayList(
new SimpleRecord(1, "a"),
new SimpleRecord(2, "b"),
new SimpleRecord(3, "c")
);
Dataset<Row> firstDf = spark.createDataFrame(firstBatchRecords, SimpleRecord.class);
firstDf.select("id", "data").write().format("iceberg").mode("append").save(tableLocation);

table.manageSnapshots().createTag("tag", table.currentSnapshot().snapshotId()).commit();

// produce the second snapshot
List<SimpleRecord> secondBatchRecords = Lists.newArrayList(
new SimpleRecord(4, "d"),
new SimpleRecord(5, "e"),
new SimpleRecord(6, "f")
);
Dataset<Row> secondDf = spark.createDataFrame(secondBatchRecords, SimpleRecord.class);
secondDf.select("id", "data").write().format("iceberg").mode("append").save(tableLocation);

// verify records in the current snapshot by tag
Dataset<Row> currentSnapshotResult = spark.read()
.format("iceberg")
.option("tag", "tag")
.load(tableLocation);
currentSnapshotResult.show();
List<SimpleRecord> currentSnapshotRecords = currentSnapshotResult.orderBy("id")
.as(Encoders.bean(SimpleRecord.class))
.collectAsList();
List<SimpleRecord> expectedRecords = Lists.newArrayList();
expectedRecords.addAll(firstBatchRecords);
Assert.assertEquals("Current snapshot rows should match", expectedRecords, currentSnapshotRecords);
}

@Test
public void testSnapshotSelectionByBranch() throws IOException {
String tableLocation = temp.newFolder("iceberg-table").toString();

HadoopTables tables = new HadoopTables(CONF);
PartitionSpec spec = PartitionSpec.unpartitioned();
Table table = tables.create(SCHEMA, spec, tableLocation);

// produce the first snapshot
List<SimpleRecord> firstBatchRecords = Lists.newArrayList(
new SimpleRecord(1, "a"),
new SimpleRecord(2, "b"),
new SimpleRecord(3, "c")
);
Dataset<Row> firstDf = spark.createDataFrame(firstBatchRecords, SimpleRecord.class);
firstDf.select("id", "data").write().format("iceberg").mode("append").save(tableLocation);

table.manageSnapshots().createBranch("branch", table.currentSnapshot().snapshotId()).commit();

// produce the second snapshot
List<SimpleRecord> secondBatchRecords = Lists.newArrayList(
new SimpleRecord(4, "d"),
new SimpleRecord(5, "e"),
new SimpleRecord(6, "f")
);
Dataset<Row> secondDf = spark.createDataFrame(secondBatchRecords, SimpleRecord.class);
secondDf.select("id", "data").write().format("iceberg").mode("append").save(tableLocation);

// verify records in the current snapshot by tag
Dataset<Row> currentSnapshotResult = spark.read()
.format("iceberg")
.option("branch", "branch")
.load(tableLocation);
currentSnapshotResult.show();
List<SimpleRecord> currentSnapshotRecords = currentSnapshotResult.orderBy("id")
.as(Encoders.bean(SimpleRecord.class))
.collectAsList();
List<SimpleRecord> expectedRecords = Lists.newArrayList();
expectedRecords.addAll(firstBatchRecords);
Assert.assertEquals("Current snapshot rows should match", expectedRecords, currentSnapshotRecords);
}
}