amogh-jahagirdar commented on code in PR #6717:
URL: https://github.com/apache/iceberg/pull/6717#discussion_r1103738003
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCachedTableCatalog.java:
##########
@@ -144,23 +149,49 @@ private Pair<Table, Long> load(Identifier ident) throws
NoSuchTableException {
if (snapshotBasedMatcher.matches()) {
snapshotId = Long.parseLong(snapshotBasedMatcher.group(1));
}
+
+ Matcher branchBasedMatcher = BRANCH.matcher(meta);
+ if (branchBasedMatcher.matches()) {
+ branch = branchBasedMatcher.group(1);
+ }
+
+ Matcher tagBasedMatcher = BRANCH.matcher(meta);
+ if (tagBasedMatcher.matches()) {
+ tag = tagBasedMatcher.group(1);
+ }
}
Preconditions.checkArgument(
- asOfTimestamp == null || snapshotId == null,
- "Cannot specify both snapshot and timestamp for time travel: %s",
- ident);
+ Stream.of(snapshotId, asOfTimestamp, branch,
tag).filter(Objects::nonNull).count() <= 1,
+ "Can specify at most one of snapshot-id (%s), as-of-timestamp (%s),
and snapshot-ref (%s)",
+ snapshotId,
+ asOfTimestamp,
+ branch);
Table table = TABLE_CACHE.get(key);
if (table == null) {
throw new NoSuchTableException(ident);
}
+ long snapshotIdFromTimeTravel = -1L;
+
if (snapshotId != null) {
- return Pair.of(table, snapshotId);
+ snapshotIdFromTimeTravel = snapshotId;
} else if (asOfTimestamp != null) {
- return Pair.of(table, SnapshotUtil.snapshotIdAsOfTime(table,
asOfTimestamp));
+ snapshotIdFromTimeTravel = SnapshotUtil.snapshotIdAsOfTime(table,
asOfTimestamp);
+ } else if (branch != null) {
+ Preconditions.checkArgument(
+ table.snapshot(branch) != null, "branch not associated with a
snapshot");
+ snapshotIdFromTimeTravel = table.snapshot(branch).snapshotId();
+ } else if (tag != null) {
+ Preconditions.checkArgument(
+ table.snapshot(tag) != null, "tag not associated with a snapshot");
Review Comment:
Same comment as @jackye1995 let's include the ref name in the error itself
so it's obvious. `Cannot find snapshot associated with tag: %s`
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCachedTableCatalog.java:
##########
@@ -144,23 +149,49 @@ private Pair<Table, Long> load(Identifier ident) throws
NoSuchTableException {
if (snapshotBasedMatcher.matches()) {
snapshotId = Long.parseLong(snapshotBasedMatcher.group(1));
}
+
+ Matcher branchBasedMatcher = BRANCH.matcher(meta);
+ if (branchBasedMatcher.matches()) {
+ branch = branchBasedMatcher.group(1);
+ }
+
+ Matcher tagBasedMatcher = BRANCH.matcher(meta);
Review Comment:
Should be `TAG` instead of `BRANCH ` here
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/SparkCachedTableCatalog.java:
##########
@@ -144,23 +149,49 @@ private Pair<Table, Long> load(Identifier ident) throws
NoSuchTableException {
if (snapshotBasedMatcher.matches()) {
snapshotId = Long.parseLong(snapshotBasedMatcher.group(1));
}
+
+ Matcher branchBasedMatcher = BRANCH.matcher(meta);
+ if (branchBasedMatcher.matches()) {
+ branch = branchBasedMatcher.group(1);
+ }
+
+ Matcher tagBasedMatcher = BRANCH.matcher(meta);
+ if (tagBasedMatcher.matches()) {
+ tag = tagBasedMatcher.group(1);
+ }
}
Preconditions.checkArgument(
- asOfTimestamp == null || snapshotId == null,
- "Cannot specify both snapshot and timestamp for time travel: %s",
- ident);
+ Stream.of(snapshotId, asOfTimestamp, branch,
tag).filter(Objects::nonNull).count() <= 1,
+ "Can specify at most one of snapshot-id (%s), as-of-timestamp (%s),
and snapshot-ref (%s)",
+ snapshotId,
+ asOfTimestamp,
+ branch);
Table table = TABLE_CACHE.get(key);
if (table == null) {
throw new NoSuchTableException(ident);
}
+ long snapshotIdFromTimeTravel = -1L;
Review Comment:
Ah yeah, most of the duplication comes from having the different time travel
options but ultimately we end up having to have a branching logic anyways to
assign the snapshot id. I'm good to revert back to the existing way, we should
generally strive to minimize changes as long as it's still readable which it is
in the existing case as well. sorry for the confusion there @namrathamyske !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
