Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 @@ -109,7 +109,7 @@ public void commit(TableMetadata base, TableMetadata metadata) {
// if the metadata is already out of date, reject it
if (base != current()) {
if (base != null) {
throw new CommitFailedException("Cannot commit: stale table metadata");
throw new CommitFailedException("Commit failed: stale table metadata");
} else {
// when current is non-null, the table exists. but when base is null, the commit is trying
// to create the table
Expand Down
51 changes: 51 additions & 0 deletions core/src/main/java/org/apache/iceberg/UpdateRequirement.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.apache.iceberg;

import java.util.Optional;
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
Expand Down Expand Up @@ -230,4 +231,54 @@ public void validate(TableMetadata base) {
}
}
}

class AssertCurrentViewVersionID implements UpdateRequirement {
Copy link
Contributor

Choose a reason for hiding this comment

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

here's some historical context on why I didn't add it back then when views were introduced: #8147 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comes down to whether we have a good enough argument for making a "replace view" fail

private final int viewVersionId;

public AssertCurrentViewVersionID(int viewVersionId) {
this.viewVersionId = viewVersionId;
}

public int viewVersionId() {
return viewVersionId;
}

@Override
public void validate(ViewMetadata base) {
if (viewVersionId != base.currentVersionId()) {
throw new CommitFailedException(
"Requirement failed: current view version changed: expected version %s != %s",
viewVersionId, base.currentVersionId());
}
}
}

/**
* Assuming that view ID is incrementing integers, so the last assigned view version ID is always
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't something that holds true. View versions are not guaranteed to be monotonically increasing and the spec doesn't say anything about this

* the max ID that has been assigned.
*/
class AssertLastAssignedViewVersionID implements UpdateRequirement {
Copy link
Contributor

Choose a reason for hiding this comment

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

here's some historical context on why I didn't add it back then when introducing views: #8147 (comment)

private final int lastAssignedViewVersionId;

public AssertLastAssignedViewVersionID(int lastAssignedViewVersionId) {
this.lastAssignedViewVersionId = lastAssignedViewVersionId;
}

public int lastAssignedViewId() {
return lastAssignedViewVersionId;
}

@Override
public void validate(ViewMetadata base) {
Optional<Integer> maxAssignedViewVersionID =
base.versionsById().keySet().stream().max(Integer::compareTo);
if (base != null
&& maxAssignedViewVersionID.isPresent()
&& maxAssignedViewVersionID.get() != lastAssignedViewVersionId) {
throw new CommitFailedException(
"Requirement failed: last assigned view id changed: expected id %s != %s",
lastAssignedViewVersionId, maxAssignedViewVersionID.get());
}
}
}
}
46 changes: 46 additions & 0 deletions core/src/main/java/org/apache/iceberg/UpdateRequirementParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ private UpdateRequirementParser() {}
static final String ASSERT_LAST_ASSIGNED_PARTITION_ID = "assert-last-assigned-partition-id";
static final String ASSERT_DEFAULT_SPEC_ID = "assert-default-spec-id";
static final String ASSERT_DEFAULT_SORT_ORDER_ID = "assert-default-sort-order-id";
static final String ASSERT_CURRENT_VIEW_VERSION_ID = "assert-current-view-version-id";
static final String ASSERT_LAST_ASSIGNED_VIEW_VERSION_ID = "assert-last-assigned-view-version-id";

// AssertTableUUID
private static final String UUID = "uuid";
Expand All @@ -66,6 +68,12 @@ private UpdateRequirementParser() {}
// AssertDefaultSortOrderID
private static final String SORT_ORDER_ID = "default-sort-order-id";

// AssertCurrentViewVersionID
private static final String CURRENT_VIEW_VERSION_ID = "current-view-version-id";

// AssertLastAssignedViewVersionID
private static final String LAST_ASSIGNED_VIEW_VERSION_ID = "last-assigned-view-version-id";

private static final Map<Class<? extends UpdateRequirement>, String> TYPES =
ImmutableMap.<Class<? extends UpdateRequirement>, String>builder()
.put(UpdateRequirement.AssertTableUUID.class, ASSERT_TABLE_UUID)
Expand All @@ -79,6 +87,10 @@ private UpdateRequirementParser() {}
ASSERT_LAST_ASSIGNED_PARTITION_ID)
.put(UpdateRequirement.AssertDefaultSpecID.class, ASSERT_DEFAULT_SPEC_ID)
.put(UpdateRequirement.AssertDefaultSortOrderID.class, ASSERT_DEFAULT_SORT_ORDER_ID)
.put(UpdateRequirement.AssertCurrentViewVersionID.class, ASSERT_CURRENT_VIEW_VERSION_ID)
.put(
UpdateRequirement.AssertLastAssignedViewVersionID.class,
ASSERT_LAST_ASSIGNED_VIEW_VERSION_ID)
.buildOrThrow();

public static String toJson(UpdateRequirement updateRequirement) {
Expand Down Expand Up @@ -130,6 +142,14 @@ public static void toJson(UpdateRequirement updateRequirement, JsonGenerator gen
writeAssertDefaultSortOrderId(
(UpdateRequirement.AssertDefaultSortOrderID) updateRequirement, generator);
break;
case ASSERT_CURRENT_VIEW_VERSION_ID:
writeAssertCurrentViewVersionId(
(UpdateRequirement.AssertCurrentViewVersionID) updateRequirement, generator);
break;
case ASSERT_LAST_ASSIGNED_VIEW_VERSION_ID:
writeAssertLastAssignedViewVersionId(
(UpdateRequirement.AssertLastAssignedViewVersionID) updateRequirement, generator);
break;
default:
throw new IllegalArgumentException(
String.format(
Expand Down Expand Up @@ -178,6 +198,10 @@ public static UpdateRequirement fromJson(JsonNode jsonNode) {
return readAssertDefaultSpecId(jsonNode);
case ASSERT_DEFAULT_SORT_ORDER_ID:
return readAssertDefaultSortOrderId(jsonNode);
case ASSERT_CURRENT_VIEW_VERSION_ID:
return readAssertCurrentViewVersionId(jsonNode);
case ASSERT_LAST_ASSIGNED_VIEW_VERSION_ID:
return readAssertLastAssignedViewVersionId(jsonNode);
default:
throw new UnsupportedOperationException(
String.format("Unrecognized update requirement. Cannot convert to json: %s", type));
Expand Down Expand Up @@ -232,6 +256,18 @@ private static void writeAssertDefaultSortOrderId(
gen.writeNumberField(SORT_ORDER_ID, requirement.sortOrderId());
}

private static void writeAssertCurrentViewVersionId(
UpdateRequirement.AssertCurrentViewVersionID requirement, JsonGenerator gen)
throws IOException {
gen.writeNumberField(CURRENT_VIEW_VERSION_ID, requirement.viewVersionId());
}

private static void writeAssertLastAssignedViewVersionId(
UpdateRequirement.AssertLastAssignedViewVersionID requirement, JsonGenerator gen)
throws IOException {
gen.writeNumberField(LAST_ASSIGNED_VIEW_VERSION_ID, requirement.lastAssignedViewId());
}

@SuppressWarnings(
"unused") // Keep same signature in case this requirement class evolves and gets fields
private static UpdateRequirement readAssertTableDoesNotExist(JsonNode node) {
Expand Down Expand Up @@ -278,4 +314,14 @@ private static UpdateRequirement readAssertDefaultSortOrderId(JsonNode node) {
int sortOrderId = JsonUtil.getInt(SORT_ORDER_ID, node);
return new UpdateRequirement.AssertDefaultSortOrderID(sortOrderId);
}

private static UpdateRequirement readAssertCurrentViewVersionId(JsonNode node) {
int viewVersionId = JsonUtil.getInt(CURRENT_VIEW_VERSION_ID, node);
return new UpdateRequirement.AssertCurrentViewVersionID(viewVersionId);
}

private static UpdateRequirement readAssertLastAssignedViewVersionId(JsonNode node) {
int viewVersionId = JsonUtil.getInt(LAST_ASSIGNED_VIEW_VERSION_ID, node);
return new UpdateRequirement.AssertLastAssignedViewVersionID(viewVersionId);
}
}
71 changes: 52 additions & 19 deletions core/src/main/java/org/apache/iceberg/UpdateRequirements.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ private UpdateRequirements() {}

public static List<UpdateRequirement> forCreateTable(List<MetadataUpdate> metadataUpdates) {
Preconditions.checkArgument(null != metadataUpdates, "Invalid metadata updates: null");
Builder builder = new Builder(null, false);
Builder builder = new Builder((TableMetadata) null, false);
builder.require(new UpdateRequirement.AssertTableDoesNotExist());
metadataUpdates.forEach(builder::update);
return builder.build();
Expand Down Expand Up @@ -61,14 +61,15 @@ public static List<UpdateRequirement> forReplaceView(
ViewMetadata base, List<MetadataUpdate> metadataUpdates) {
Preconditions.checkArgument(null != base, "Invalid view metadata: null");
Preconditions.checkArgument(null != metadataUpdates, "Invalid metadata updates: null");
Builder builder = new Builder(null, false);
Builder builder = new Builder(base, false);
builder.require(new UpdateRequirement.AssertViewUUID(base.uuid()));
metadataUpdates.forEach(builder::update);
return builder.build();
}

private static class Builder {
private final TableMetadata base;
private final TableMetadata baseTable;
private final ViewMetadata baseView;
private final ImmutableList.Builder<UpdateRequirement> requirements = ImmutableList.builder();
private final Set<String> changedRefs = Sets.newHashSet();
private final boolean isReplace;
Expand All @@ -78,8 +79,15 @@ private static class Builder {
private boolean setSpecId = false;
private boolean setOrderId = false;

private Builder(TableMetadata base, boolean isReplace) {
this.base = base;
private Builder(TableMetadata baseTable, boolean isReplace) {
this.baseTable = baseTable;
this.baseView = null;
this.isReplace = isReplace;
}

private Builder(ViewMetadata baseView, boolean isReplace) {
this.baseTable = null;
this.baseView = baseView;
this.isReplace = isReplace;
}

Expand Down Expand Up @@ -110,6 +118,13 @@ private Builder update(MetadataUpdate update) {
} else if (update instanceof MetadataUpdate.RemoveSchemas) {
update((MetadataUpdate.RemoveSchemas) update);
}
// the above handles requirement for table updates
// the below handles requirement for view updates
else if (update instanceof MetadataUpdate.AddViewVersion) {
update((MetadataUpdate.AddViewVersion) update);
} else if (update instanceof MetadataUpdate.SetCurrentViewVersion) {
update((MetadataUpdate.SetCurrentViewVersion) update);
}

return this;
}
Expand All @@ -119,8 +134,8 @@ private void update(MetadataUpdate.SetSnapshotRef setRef) {
String name = setRef.name();
// add returns true the first time the ref name is added
boolean added = changedRefs.add(name);
if (added && base != null && !isReplace) {
SnapshotRef baseRef = base.ref(name);
if (added && baseTable != null && !isReplace) {
SnapshotRef baseRef = baseTable.ref(name);
// require that the ref does not exist (null) or is the same as the base snapshot
require(
new UpdateRequirement.AssertRefSnapshotID(
Expand All @@ -130,8 +145,8 @@ private void update(MetadataUpdate.SetSnapshotRef setRef) {

private void update(MetadataUpdate.AddSchema unused) {
if (!addedSchema) {
if (base != null) {
require(new UpdateRequirement.AssertLastAssignedFieldId(base.lastColumnId()));
if (baseTable != null) {
require(new UpdateRequirement.AssertLastAssignedFieldId(baseTable.lastColumnId()));
}
this.addedSchema = true;
}
Expand All @@ -143,9 +158,10 @@ private void update(MetadataUpdate.SetCurrentSchema unused) {

private void update(MetadataUpdate.AddPartitionSpec unused) {
if (!addedSpec) {
if (base != null) {
if (baseTable != null) {
require(
new UpdateRequirement.AssertLastAssignedPartitionId(base.lastAssignedPartitionId()));
new UpdateRequirement.AssertLastAssignedPartitionId(
baseTable.lastAssignedPartitionId()));
}
this.addedSpec = true;
}
Expand All @@ -157,9 +173,9 @@ private void update(MetadataUpdate.SetDefaultPartitionSpec unused) {

private void update(MetadataUpdate.SetDefaultSortOrder unused) {
if (!setOrderId) {
if (base != null && !isReplace) {
if (baseTable != null && !isReplace) {
// require that the default write order has not changed
require(new UpdateRequirement.AssertDefaultSortOrderID(base.defaultSortOrderId()));
require(new UpdateRequirement.AssertDefaultSortOrderID(baseTable.defaultSortOrderId()));
}
this.setOrderId = true;
}
Expand All @@ -179,27 +195,44 @@ private void update(MetadataUpdate.RemoveSchemas unused) {
requireNoBranchesChanged();
}

private void update(MetadataUpdate.AddViewVersion unused) {
Preconditions.checkArgument(baseView != null, "Base view metadata is required");

baseView.versionsById().keySet().stream()
.max(Integer::compareTo)
.ifPresent(
viewVersion ->
require(new UpdateRequirement.AssertLastAssignedViewVersionID(viewVersion)));
}

private void update(MetadataUpdate.SetCurrentViewVersion unused) {
Preconditions.checkArgument(baseView != null, "Base view metadata is required");

require(new UpdateRequirement.AssertCurrentViewVersionID(baseView.currentVersionId()));
}

private void requireDefaultPartitionSpecNotChanged() {
if (!setSpecId) {
if (base != null && !isReplace) {
require(new UpdateRequirement.AssertDefaultSpecID(base.defaultSpecId()));
if (baseTable != null && !isReplace) {
require(new UpdateRequirement.AssertDefaultSpecID(baseTable.defaultSpecId()));
}
this.setSpecId = true;
}
}

private void requireCurrentSchemaNotChanged() {
if (!setSchemaId) {
if (base != null && !isReplace) {
require(new UpdateRequirement.AssertCurrentSchemaID(base.currentSchemaId()));
if (baseTable != null && !isReplace) {
require(new UpdateRequirement.AssertCurrentSchemaID(baseTable.currentSchemaId()));
}
this.setSchemaId = true;
}
}

private void requireNoBranchesChanged() {
if (base != null && !isReplace) {
base.refs()
if (baseTable != null && !isReplace) {
baseTable
.refs()
.forEach(
(name, ref) -> {
if (ref.isBranch() && !name.equals(SnapshotRef.MAIN_BRANCH)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void commit(ViewMetadata base, ViewMetadata metadata) {
// if the metadata is already out of date, reject it
if (base != current()) {
if (base != null) {
throw new CommitFailedException("Cannot commit: stale view metadata");
throw new CommitFailedException("Commit failed: stale view metadata");
Copy link
Contributor

Choose a reason for hiding this comment

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

this and the other one seem like unrelated changes?

} else {
// when current is non-null, the view exists. but when base is null, the commit is trying
// to create the view
Expand Down
Loading