Skip to content

Views: Fix SQL view representation field name #7417

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 2 commits into from
Apr 25, 2023
Merged

Views: Fix SQL view representation field name #7417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b222790
Views: Miscellaneous updates.
rdblue Apr 23, 2023
ed4091a
Views: Use field-comments.
rdblue Apr 24, 2023
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
5 changes: 4 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 @@ -65,5 +65,8 @@ public interface ViewVersion {
*
* @return the string operation which produced the view version
*/
String operation();
@Value.Derived
default String operation() {
return summary().get("operation");
}
}
Comment on lines +68 to 72
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Apr 24, 2023

Choose a reason for hiding this comment

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

Nice, Derived is way cleaner. We don't need any of the special parsing logic for operation and derived also prevents manually setting the operation field, so there is no ambiguity in which is the real "operation"

6 changes: 2 additions & 4 deletions core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,11 @@ static void toJson(SQLViewRepresentation view, JsonGenerator generator) throws I
}

if (!view.fieldAliases().isEmpty()) {
JsonUtil.writeStringArray(
SQLViewRepresentationParser.FIELD_ALIASES, view.fieldAliases(), generator);
JsonUtil.writeStringArray(FIELD_ALIASES, view.fieldAliases(), generator);
}

if (!view.fieldComments().isEmpty()) {
JsonUtil.writeStringArray(
SQLViewRepresentationParser.FIELD_COMMENTS, view.fieldComments(), generator);
JsonUtil.writeStringArray(FIELD_COMMENTS, view.fieldComments(), generator);
}

generator.writeEndObject();
Expand Down
7 changes: 4 additions & 3 deletions core/src/main/java/org/apache/iceberg/view/ViewRepresentationParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@ private ViewRepresentationParser() {}
static void toJson(ViewRepresentation representation, JsonGenerator generator)
throws IOException {
Preconditions.checkArgument(representation != null, "Invalid view representation: null");
switch (representation.type()) {
switch (representation.type().toLowerCase(Locale.ENGLISH)) {
case ViewRepresentation.Type.SQL:
SQLViewRepresentationParser.toJson((SQLViewRepresentation) representation, generator);
break;

default:
throw new IllegalArgumentException(
String.format("Cannot serialize view representation type: %s", representation.type()));
throw new UnsupportedOperationException(
String.format(
"Cannot serialize unsupported view representation: %s", representation.type()));
}
}

Expand Down
26 changes: 5 additions & 21 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 @@ -24,7 +24,6 @@
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.util.JsonUtil;

class ViewVersionParser {
Expand All @@ -40,24 +39,25 @@ private ViewVersionParser() {}
static void toJson(ViewVersion version, JsonGenerator generator) throws IOException {
Preconditions.checkArgument(version != null, "Cannot serialize null view version");
generator.writeStartObject();

generator.writeNumberField(VERSION_ID, version.versionId());
generator.writeNumberField(TIMESTAMP_MS, version.timestampMillis());

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();

generator.writeArrayFieldStart(REPRESENTATIONS);
for (ViewRepresentation representation : version.representations()) {
ViewRepresentationParser.toJson(representation, generator);
}

generator.writeEndArray();

generator.writeEndObject();
}

Expand All @@ -80,22 +80,7 @@ static ViewVersion fromJson(JsonNode node) {
long timestamp = JsonUtil.getLong(TIMESTAMP_MS, node);
Map<String, String> summary = JsonUtil.getStringMap(SUMMARY, node);
Preconditions.checkArgument(
summary != null, "Cannot parse view version with missing required field: %s", SUMMARY);

String operation = null;
ImmutableMap.Builder<String, String> versionSummary = ImmutableMap.builder();
for (Map.Entry<String, String> summaryEntry : summary.entrySet()) {
if (summaryEntry.getKey().equals(OPERATION)) {
operation = summaryEntry.getValue();
} else {
versionSummary.put(summaryEntry.getKey(), summaryEntry.getValue());
}
}

Preconditions.checkArgument(
operation != null,
"Cannot parse view version summary with missing required field: %s",
OPERATION);
summary.containsKey(OPERATION), "Invalid view version summary, missing %s", OPERATION);
Copy link
Contributor

@nastra nastra Apr 25, 2023

Choose a reason for hiding this comment

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

one issue with this is unfortunately that we're only checking for the existence of operation in the Parser. With Immutables you'd typically do this on the constructed object itself during initialization using a method annotated with @Value.Check to guarantee a consistent state.
For example, with the current version one could create a ViewVersion without an operation in the summary map, which would fail with a confusing error: java.lang.NullPointerException: operation.

@rdblue I have opened #7428 to move the check to ViewVersion


JsonNode serializedRepresentations = node.get(REPRESENTATIONS);
ImmutableList.Builder<ViewRepresentation> representations = ImmutableList.builder();
Expand All @@ -108,8 +93,7 @@ static ViewVersion fromJson(JsonNode node) {
return ImmutableViewVersion.builder()
.versionId(versionId)
.timestampMillis(timestamp)
.summary(versionSummary.build())
.operation(operation)
.summary(summary)
.representations(representations.build())
.build();
}
Expand Down
4 changes: 2 additions & 2 deletions core/src/test/java/org/apache/iceberg/view/TestViewRepresentationParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ public void testParseUnknownViewRepresentation() {
ImmutableUnknownViewRepresentation.builder().type("unknown-sql-representation").build());

Assertions.assertThatThrownBy(() -> ViewRepresentationParser.toJson(unknownRepresentation))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot serialize view representation type: unknown-sql-representation");
.isInstanceOf(UnsupportedOperationException.class)
.hasMessage("Cannot serialize unsupported view representation: unknown-sql-representation");
}

@Test
Expand Down
8 changes: 3 additions & 5 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 @@ -44,8 +44,7 @@ public void testParseViewVersion() {
.versionId(1)
.timestampMillis(12345)
.addRepresentations(firstRepresentation, secondRepresentation)
.operation("create")
.summary(ImmutableMap.of("user", "some-user"))
.summary(ImmutableMap.of("operation", "create", "user", "some-user"))
.build();

String serializedRepresentations =
Expand Down Expand Up @@ -81,8 +80,7 @@ public void testSerializeViewVersion() {
.versionId(1)
.timestampMillis(12345)
.addRepresentations(firstRepresentation, secondRepresentation)
.summary(ImmutableMap.of("user", "some-user"))
.operation("create")
.summary(ImmutableMap.of("operation", "create", "user", "some-user"))
.build();

String expectedRepresentations =
Expand Down Expand Up @@ -113,7 +111,7 @@ public void testFailParsingMissingOperation() {

Assertions.assertThatThrownBy(() -> ViewVersionParser.fromJson(viewVersionMissingOperation))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse view version summary with missing required field: operation");
.hasMessage("Invalid view version summary, missing operation");
}

@Test
Expand Down