nastra commented on code in PR #6861: URL: https://github.com/apache/iceberg/pull/6861#discussion_r1109381908
########## core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.view; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonNode; +import java.io.IOException; +import java.util.Locale; +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.util.JsonUtil; + +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 ViewVersionParser() {} + + static void toJson(ViewVersion version, JsonGenerator generator) throws IOException { + Preconditions.checkArgument(version != null, "Invalid view version: null"); + generator.writeStartObject(); + generator.writeNumberField(VERSION_ID, version.versionId()); + generator.writeNumberField(TIMESTAMP_MS, version.timestampMillis()); + JsonUtil.writeStringMap(SUMMARY, version.summary(), generator); + + generator.writeArrayFieldStart(REPRESENTATIONS); + for (ViewRepresentation representation : version.representations()) { + ViewRepresentationParser.toJson(representation, generator); + } + generator.writeEndArray(); + + generator.writeEndObject(); + } + + static String toJson(ViewVersion version) { + return JsonUtil.generate(gen -> toJson(version, gen), false); + } + + static ViewVersion fromJson(String json) { + return JsonUtil.parse(json, ViewVersionParser::fromJson); + } + + static ViewVersion fromJson(JsonNode node) { + Preconditions.checkArgument(node != null, "Cannot parse view version from null object"); + + Preconditions.checkArgument( + node.isObject(), "Cannot parse table version from a non-object: %s", node); + + int versionId = JsonUtil.getInt(VERSION_ID, node); + long timestamp = JsonUtil.getLong(TIMESTAMP_MS, node); + Map<String, String> summary = JsonUtil.getStringMap(SUMMARY, node); + String operation = summary.get(OPERATION); + Preconditions.checkArgument( + operation != null, "Cannot parse summary with missing required field: %s", OPERATION); + + try { + ViewVersion.Operation.valueOf(operation.toUpperCase(Locale.ENGLISH)); Review Comment: what we have done in other enums across the codebase is to have a `fromName()` method on the enum itself and do the parsing there, such as in https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/DistributionMode.java#L54-L61 or in https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/FileFormat.java#L70. I think that would be good to have in the `Operation` enum as well. ########## core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.view; + +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.assertj.core.api.Assertions; +import org.junit.Assert; +import org.junit.Test; + +public class TestViewVersionParser { + + @Test + public void testParseViewVersion() { + SQLViewRepresentation firstRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select * from foo") + .dialect("spark-sql") + .build(); + SQLViewRepresentation secondRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select a, b, c from foo") + .dialect("some-sql") + .build(); + + ViewVersion expectedViewVersion = + ImmutableViewVersion.builder() + .versionId(1) + .timestampMillis(12345) + .addRepresentations(firstRepresentation, secondRepresentation) + .summary(ImmutableMap.of("operation", "CREATE")) + .build(); + + String serializedRepresentations = + "[{\"type\":\"sql\", \"sql\":\"select * from foo\", \"dialect\":\"spark-sql\"}, " + + "{\"type\":\"sql\", \"sql\":\"select a, b, c from foo\", \"dialect\":\"some-sql\"}]"; + + String serializedViewVersion = + String.format( + "{\"version-id\":1, \"timestamp-ms\":12345, \"summary\":{\"operation\":\"CREATE\"}, \"representations\":%s}", + serializedRepresentations); + + Assert.assertEquals( + "Should be able to parse valid view version", + expectedViewVersion, + ViewVersionParser.fromJson(serializedViewVersion)); + } + + @Test + public void testSerializeViewVersion() { + + SQLViewRepresentation firstRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select * from foo") + .dialect("spark-sql") + .build(); + SQLViewRepresentation secondRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select a, b, c from foo") + .dialect("some-sql") + .build(); + + ViewVersion viewVersion = + ImmutableViewVersion.builder() + .versionId(1) + .timestampMillis(12345) + .addRepresentations(firstRepresentation, secondRepresentation) + .summary(ImmutableMap.of("operation", "CREATE")) + .build(); + + String expectedRepresentations = + "[{\"type\":\"sql\",\"sql\":\"select * from foo\",\"dialect\":\"spark-sql\"}," + + "{\"type\":\"sql\",\"sql\":\"select a, b, c from foo\",\"dialect\":\"some-sql\"}]"; + + String expectedViewVersion = + String.format( + "{\"version-id\":1,\"timestamp-ms\":12345,\"summary\":{\"operation\":\"CREATE\"},\"representations\":%s}", + expectedRepresentations); + + Assert.assertEquals( + "Should be able to parse valid view version", + expectedViewVersion, + ViewVersionParser.toJson(viewVersion)); + } + + @Test + public void testFailParsingMissingOperation() { + String serializedRepresentations = + "[{\"type\":\"sql\",\"sql\":\"select * from foo\",\"dialect\":\"spark-sql\"}," + + "{\"type\":\"sql\",\"sql\":\"select a, b, c from foo\",\"dialect\":\"some-sql\"}]"; + + String viewVersionMissingOperation = + String.format( + "{\"version-id\":1,\"timestamp-ms\":12345,\"summary\":{\"some-other-field\":\"some-other-value\"},\"representations\":%s}", + serializedRepresentations); + + Assertions.assertThatThrownBy(() -> ViewVersionParser.fromJson(viewVersionMissingOperation)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse summary with missing required field: operation"); + } + + @Test + public void testFailParsingInvalidOperation() { + String serializedRepresentations = + "[{\"type\":\"sql\",\"sql\":\"select * from foo\",\"dialect\":\"spark-sql\"}," + + "{\"type\":\"sql\",\"sql\":\"select a, b, c from foo\",\"dialect\":\"some-sql\"}]"; + + String viewVersionMissingOperation = + String.format( + "{\"version-id\":1,\"timestamp-ms\":12345,\"summary\":{\"operation\":\"unknown-operation\"},\"representations\":%s}", + serializedRepresentations); + + Assertions.assertThatThrownBy(() -> ViewVersionParser.fromJson(viewVersionMissingOperation)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Cannot parse summary with invalid operation: unknown-operation"); + } + + @Test + public void testNullViewVersion() { + Assertions.assertThatThrownBy(() -> ViewVersionParser.toJson(null)) Review Comment: would be good to also add a test with a null string here: `Assertions.assertThatThrownBy(() -> ViewVersionParser.toJson((String) null))` ########## core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.view; + +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.assertj.core.api.Assertions; +import org.junit.Assert; +import org.junit.Test; + +public class TestViewVersionParser { + + @Test + public void testParseViewVersion() { + SQLViewRepresentation firstRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select * from foo") + .dialect("spark-sql") + .build(); + SQLViewRepresentation secondRepresentation = + ImmutableSQLViewRepresentation.builder() + .sql("select a, b, c from foo") + .dialect("some-sql") + .build(); + + ViewVersion expectedViewVersion = + ImmutableViewVersion.builder() + .versionId(1) + .timestampMillis(12345) + .addRepresentations(firstRepresentation, secondRepresentation) + .summary(ImmutableMap.of("operation", "CREATE")) + .build(); + + String serializedRepresentations = + "[{\"type\":\"sql\", \"sql\":\"select * from foo\", \"dialect\":\"spark-sql\"}, " + + "{\"type\":\"sql\", \"sql\":\"select a, b, c from foo\", \"dialect\":\"some-sql\"}]"; + + String serializedViewVersion = + String.format( + "{\"version-id\":1, \"timestamp-ms\":12345, \"summary\":{\"operation\":\"CREATE\"}, \"representations\":%s}", + serializedRepresentations); + + Assert.assertEquals( + "Should be able to parse valid view version", + expectedViewVersion, + ViewVersionParser.fromJson(serializedViewVersion)); + } + + @Test + public void testSerializeViewVersion() { + Review Comment: nit: empty line ########## core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.view; + +import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.databind.JsonNode; +import java.io.IOException; +import java.util.Locale; +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.util.JsonUtil; + +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 ViewVersionParser() {} + + static void toJson(ViewVersion version, JsonGenerator generator) throws IOException { + Preconditions.checkArgument(version != null, "Invalid view version: null"); + generator.writeStartObject(); + generator.writeNumberField(VERSION_ID, version.versionId()); + generator.writeNumberField(TIMESTAMP_MS, version.timestampMillis()); + JsonUtil.writeStringMap(SUMMARY, version.summary(), generator); + + generator.writeArrayFieldStart(REPRESENTATIONS); + for (ViewRepresentation representation : version.representations()) { + ViewRepresentationParser.toJson(representation, generator); + } + generator.writeEndArray(); + + generator.writeEndObject(); + } + + static String toJson(ViewVersion version) { + return JsonUtil.generate(gen -> toJson(version, gen), false); + } + + static ViewVersion fromJson(String json) { + return JsonUtil.parse(json, ViewVersionParser::fromJson); + } + + static ViewVersion fromJson(JsonNode node) { + Preconditions.checkArgument(node != null, "Cannot parse view version from null object"); + + Preconditions.checkArgument( + node.isObject(), "Cannot parse table version from a non-object: %s", node); Review Comment: ```suggestion node.isObject(), "Cannot parse view version from a non-object: %s", node); ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
