Skip to content

Views: Move 'operation' check to ViewVersion #7428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jackye1995 merged 1 commit into from
Apr 26, 2023
Merged

Views: Move 'operation' check to ViewVersion #7428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion api/src/main/java/org/apache/iceberg/view/ViewVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.util.List;
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.immutables.value.Value;

/**
Expand Down Expand Up @@ -65,11 +66,17 @@ public interface ViewVersion {
*
* @return the string operation which produced the view version
*/
@Value.Derived
@Value.Lazy
Copy link
Contributor Author

@nastra nastra Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@Value.Derived is eagerly computing the value before check() is executed. Switching to @Value.Lazy here so that the check() method is executed

Copy link
Contributor

@rdblue rdblue May 21, 2023

Choose a reason for hiding this comment

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

@nastra, why does this matter? All that would happen is that this is set to null and then the check method throws an exception.

default String operation() {
return summary().get("operation");
}

/** The query output schema at version create time, without aliases */
int schemaId();

@Value.Check
default void check() {
Preconditions.checkArgument(
summary().containsKey("operation"), "Invalid view version summary, missing operation");
}
}
14 changes: 1 addition & 13 deletions core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class ViewVersionParser {
private static final String VERSION_ID = "version-id";
private static final String TIMESTAMP_MS = "timestamp-ms";
private static final String SUMMARY = "summary";
private static final String OPERATION = "operation";
private static final String REPRESENTATIONS = "representations";
private static final String SCHEMA_ID = "schema-id";

Expand All @@ -44,15 +43,7 @@ static void toJson(ViewVersion version, JsonGenerator generator) throws IOExcept
generator.writeNumberField(VERSION_ID, version.versionId());
generator.writeNumberField(TIMESTAMP_MS, version.timestampMillis());
generator.writeNumberField(SCHEMA_ID, version.schemaId());

generator.writeObjectFieldStart(SUMMARY);
generator.writeStringField(OPERATION, version.operation());
for (Map.Entry<String, String> summaryEntry : version.summary().entrySet()) {
if (!summaryEntry.getKey().equals(OPERATION)) {
generator.writeStringField(summaryEntry.getKey(), summaryEntry.getValue());
}
}
generator.writeEndObject();
JsonUtil.writeStringMap(SUMMARY, version.summary(), generator);

generator.writeArrayFieldStart(REPRESENTATIONS);
for (ViewRepresentation representation : version.representations()) {
Expand All @@ -74,16 +65,13 @@ static ViewVersion fromJson(String json) {

static ViewVersion fromJson(JsonNode node) {
Preconditions.checkArgument(node != null, "Cannot parse view version from null object");

Preconditions.checkArgument(
node.isObject(), "Cannot parse view version from a non-object: %s", node);

int versionId = JsonUtil.getInt(VERSION_ID, node);
int schemaId = JsonUtil.getInt(SCHEMA_ID, node);
long timestamp = JsonUtil.getLong(TIMESTAMP_MS, node);
Map<String, String> summary = JsonUtil.getStringMap(SUMMARY, node);
Preconditions.checkArgument(
summary.containsKey(OPERATION), "Invalid view version summary, missing %s", OPERATION);

JsonNode serializedRepresentations = node.get(REPRESENTATIONS);
ImmutableList.Builder<ViewRepresentation> representations = ImmutableList.builder();
Expand Down
11 changes: 11 additions & 0 deletions core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,17 @@ public void testFailParsingMissingOperation() {
Assertions.assertThatThrownBy(() -> ViewVersionParser.fromJson(viewVersionMissingOperation))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid view version summary, missing operation");

Assertions.assertThatThrownBy(
() ->
ImmutableViewVersion.builder()
.versionId(1)
.timestampMillis(12345)
.schemaId(1)
.summary(ImmutableMap.of("user", "some-user"))
.build())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid view version summary, missing operation");
}

@Test
Expand Down