Core: View metadata implementation #6559

amogh-jahagirdar · 2023-01-11T02:44:21Z

Co-authored-by: John Zhuge [email protected]

Discussed offline with @jzhuge, builds on the core parser changes already done on #4657 but based off the latest API changes.

amogh-jahagirdar · 2023-01-11T02:44:46Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+  private final List<ViewHistoryEntry> versionLog;
+  private final List<Schema> schemas;
+  private final Map<Integer, Schema> schemasById;
+  private final String metadataFileLocation;


Shouldn't store metadataFileLocation in the metadata file, I'll remove this

amogh-jahagirdar · 2023-01-11T02:46:19Z

core/src/test/java/org/apache/iceberg/view/TestViewMetadataParser.java

+@RunWith(Parameterized.class)
+public class TestViewMetadataParser extends ParserTestBase<ViewMetadata> {
+  private static ViewVersion version1 =
+      BaseViewVersion.builder()
+          .versionId(1)
+          .timestampMillis(4353L)
+          .addRepresentation(
+              BaseSQLViewRepresentation.builder().query("select 'foo' foo").schemaId(1).build())
+          .build();
+
+  private static ViewHistoryEntry historyEntry1 = BaseViewHistoryEntry.of(4353L, 1);
+
+  private static final Schema TEST_SCHEMA =
+      new Schema(
+          1,
+          Types.NestedField.required(1, "x", Types.LongType.get()),
+          Types.NestedField.required(2, "y", Types.LongType.get(), "comment"),
+          Types.NestedField.required(3, "z", Types.LongType.get()));
+
+  private static ViewVersion version2 =
+      BaseViewVersion.builder()
+          .versionId(2)
+          .timestampMillis(5555L)


I'll add more tests for view metadata parsing. One thing I did just a bit differently than the original PR is move the JSON to a separate file in resources

amogh-jahagirdar · 2023-01-11T02:49:00Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    if (node.has(SCHEMAS)) {
+      JsonNode schemaArray = node.get(SCHEMAS);
+      Schema currentSchema = null;
+      Preconditions.checkArgument(
+          schemaArray.isArray(), "Cannot parse schemas from non-array: %s", schemaArray);
+      // current schema ID is required when the schema array is present
+      currentSchemaId = JsonUtil.getInt(CURRENT_SCHEMA_ID, node);
+      // parse the schema array
+      ImmutableList.Builder<Schema> builder = ImmutableList.builder();
+      for (JsonNode schemaNode : schemaArray) {
+        Schema schema = SchemaParser.fromJson(schemaNode);
+        if (schema.schemaId() == currentSchemaId) {
+          currentSchema = schema;
+        }
+        builder.add(schema);
+      }


The doubt I still have is I think schemas and current schema should always be set, and thus the spec should be reflected so that it's required. Maybe there's a case that we don't want schema for the entire view and it's up to individual representations. In that case in my mind for any SQL representation in the view, the schema-id should be required (which then means there must be a schema list considering we only support SQL representations). Right now it's marked as optional in the spec

jackye1995 · 2023-01-11T04:00:34Z

api/src/main/java/org/apache/iceberg/view/SQLViewRepresentation.java

@@ -41,8 +40,8 @@ default Type type() {
  /** The default namespace when the view is created. */
  Namespace defaultNamespace();

-  /** The query output schema at version create time, without aliases. */
-  Schema schema();


why change it to schema ID? If we know the schema ID, we should also be able to retrieve the Schema object

core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java

jackye1995 · 2023-01-11T06:59:08Z

I am trying see if we can break this further so that it's easier to review for a broader audience. There are some implementations of the basic interface, such as BaseViewVersion, BaseSQLViewRepresentation, BaseViewHistoryEntry, can we have a PR to first add these objects and their corresponding parsers in 3 small PRs? This would help the reviewers, also help to add tests for each feature we implement. Otherwise there is currently just 1 test for the entire view metadata.

amogh-jahagirdar · 2023-01-11T14:18:49Z

Agreed @jackye1995 we can break this down further for easier review. I'll raise the version, representation and history entry PRs separately and this one can be focused on the view metadata as a whole.

nastra

@amogh-jahagirdar thanks for working on this. I've suggested a bunch of improvements that I think will make the code much shorter to read and easier to maintain in the long run. Also I think we need to add a few more tests around nullability in the Parsers

core/src/main/java/org/apache/iceberg/util/JsonUtil.java

core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java

core/src/main/java/org/apache/iceberg/view/ViewHistoryEntryParser.java

core/src/test/java/org/apache/iceberg/view/ParserTestBase.java

core/src/main/java/org/apache/iceberg/view/ViewRepresentationParser.java

core/src/main/java/org/apache/iceberg/view/ViewVersionParser.java

amogh-jahagirdar · 2023-01-13T21:58:34Z

Thanks @nastra I'll be taking these suggestions in all the split PRs I'm raising. Agreed, more tests on nullability/missing fields are needed, and now that we use Immutable dependency in the metrics implementation, we have a good precedent to use it here as well which will simplify a lot of the boilerplate code

rdblue · 2023-01-30T16:39:42Z

core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java

+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    BaseSQLViewRepresentation that = (BaseSQLViewRepresentation) o;


Style: this method needs to have line spacing applied.

rdblue · 2023-01-30T16:40:26Z

core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java

+
+  @Override
+  public String toString() {
+    return "BaseViewDefinition{"


Can you use a helper rather than building this by hand? It's hard to read this way. We usually base these on MoreObjects.

core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

core/src/test/resources/ValidViewMetadata.json

Co-authored-by: John Zhuge <[email protected]>

nastra · 2023-05-08T06:28:02Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    internalWrite(metadata, outputFile, false);
+  }
+
+  public static void toJson(ViewMetadata metadata, JsonGenerator generator) throws IOException {


I think we should check for null on metadata + add a test

nastra · 2023-05-08T06:30:44Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+  }
+
+  public static ViewMetadata fromJson(JsonNode node) {
+    Preconditions.checkArgument(node != null, "Cannot parse view metadata from null json");


would be good to add a test for this

nastra · 2023-05-08T06:34:43Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+  static final int DEFAULT_VIEW_FORMAT_VERSION = 1;
+  static final int SUPPORTED_VIEW_FORMAT_VERSION = 1;
+
+  public static ImmutableViewMetadata.Builder buildFrom(ViewMetadata metadata) {


is this used anywhere? Also we can probably just use ImmutableViewMetadata.builder().from(metadata).xyz().build() here

or is the goal here to just create a copy? In that case we can also just do ImmutableViewMetadata.copyOf(metadata)

I guess this is trying to follow TableMetadata.buildFrom()?

where is ImmutableViewMetadata defined? I couldn't find it in this PR or existing code

@stevenzwu ImmutableViewMetadata is a generated class from the @Value.Immutable annotation. See also https://immutables.github.io/ for some additional details

nastra · 2023-05-08T06:35:23Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+    return schemasById().get(currentSchemaId());
+  }
+
+  private static Map<Integer, ViewVersion> indexVersions(List<ViewVersion> versions) {


nit: I think indexVersions and indexSchemas don't need to be static

Isn't static preferred? If someone makes a change in the future, I'd rather they be static since that is more strict. I could see changing from private to a more open visibility, in which case we would prefer static.

nastra · 2023-05-08T06:36:10Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    }
+  }
+
+  private ViewMetadataParser() {}


nit: I think this should be at the top

nastra · 2023-05-08T06:36:37Z

core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java

+import org.junit.Test;
+import org.junit.runners.Parameterized;
+
+public class TestViewMetadata {


should this be called TestViewMetadataParser instead?

nastra · 2023-05-08T06:37:40Z

core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java

+import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
+import org.junit.Assert;
+import org.junit.Test;


I think it's worth changing this new test to using JUnit5

nastra · 2023-05-08T06:39:27Z

core/src/test/java/org/apache/iceberg/view/TestViewMetadata.java

+            .currentVersionId(2)
+            .formatVersion(1)
+            .build();
+    assertSameViewMetadata(expectedViewMetadata, ViewMetadataParser.fromJson(json));


nit: assertSame usually suggests that there's some sort of identity comparison between two objects.

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

nastra · 2023-05-08T06:42:47Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    generator.writeEndObject();
+  }
+
+  static String toJson(ViewMetadata viewMetadata) {


nit: maybe move this before the other toJson(..) method

why this is not public?

Jack makes a good point. A few of these should be reviewed. In addition to this, which reasonably should be public, I think that toJson(ViewMetadata, JsonGenerator) should not be public until we need it in another package.

old

jackye1995 · 2023-05-08T17:14:37Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+            ViewProperties.VERSION_HISTORY_SIZE_DEFAULT);
+
+    Preconditions.checkArgument(
+        numVersionsToKeep >= 1, "%s must be positive", ViewProperties.VERSION_HISTORY_SIZE);


nit: add details of the received value of numVersionsToKeep in error message

jackye1995 · 2023-05-08T17:21:05Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+        .build();
+  }
+
+  static ViewMetadata fromJson(String json) {


why this is not public?

jackye1995 · 2023-05-08T17:27:47Z

core/src/test/resources/view/ValidViewMetadata.json

@@ -0,0 +1,80 @@
+{


Looks like puffin files are in path org/apache/iceberg/puffin/v1, we should follow the same pattern

rdblue · 2023-05-21T19:26:32Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+import org.immutables.value.Value;
+
+@Value.Immutable
+public abstract class ViewMetadata implements Serializable {


The other view objects are interfaces. Why does this use an abstract class? Moving between interfaces and classes is a binary incompatible change, so I think that this should be an interface instead. Is that not possible for some reason?

My guess why this is an abstract class rather than an Interface is because of indexVersions() and indexSchemas() can't be made private if we use an Interface here, meaning that they would become part of the API, which I don't think we want

rdblue · 2023-05-21T19:27:17Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    Preconditions.checkArgument(
+        formatVersion <= ViewMetadata.SUPPORTED_VIEW_FORMAT_VERSION,
+        "Cannot read unsupported version %s",
+        formatVersion);


Minor: what is the value of adding this here rather than in check()?

I agree that this should go into check(), because otherwise you could create an invalid ViewMetadata object

Agreed, @nastra fixed this in https://github.com/apache/iceberg/pull/7759/files#diff-037c118271eb576698f0829d876d5c49919eea4bd20e6b2f98b6072cfd0d4b08R133-R135 where all the validation is done in the special check method he referred to.

rdblue · 2023-05-21T19:32:08Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+      versions.add(version);
+    }
+
+    Preconditions.checkArgument(


Seems like a lot of these checks should be in the ViewMetadata class rather than here. Otherwise, you can construct invalid ViewMetadata instances.

Agreed, @nastra fixed this in https://github.com/apache/iceberg/pull/7759/files#diff-037c118271eb576698f0829d876d5c49919eea4bd20e6b2f98b6072cfd0d4b08R133-R135 where all the validation is done in the special check method he referred to.

rdblue · 2023-05-21T19:32:35Z

core/src/main/java/org/apache/iceberg/view/ViewMetadataParser.java

+    Preconditions.checkArgument(
+        numVersionsToKeep >= 1, "%s must be positive", ViewProperties.VERSION_HISTORY_SIZE);
+
+    if (versions.size() > numVersionsToKeep) {


Why do this here? I don't think it belongs in the parser.

I agree that this shouldn't be here. The better option would be to put this into a special version of a @Check method that allows normalization. This would allow to retain the number of versions that are desired in ViewMetadata directly

Agreed, @nastra fixed this in https://github.com/apache/iceberg/pull/7759/files#diff-037c118271eb576698f0829d876d5c49919eea4bd20e6b2f98b6072cfd0d4b08R133-R135 where all the validation is done in the special check method he referred to.

stevenzwu · 2023-05-14T03:48:52Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+  static final int DEFAULT_VIEW_FORMAT_VERSION = 1;
+  static final int SUPPORTED_VIEW_FORMAT_VERSION = 1;
+
+  public static ImmutableViewMetadata.Builder buildFrom(ViewMetadata metadata) {


where is ImmutableViewMetadata defined? I couldn't find it in this PR or existing code

stevenzwu · 2023-05-14T03:57:00Z

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java

+  }
+
+  private static Map<Integer, Schema> indexSchemas(List<Schema> schemas) {
+    if (schemas == null) {


could schemas be null? the spec says it is optional. but it is also says A list of schemas, the same as the ‘schemas’ field from Iceberg table spec.

It is optional to support both v1 and v2 tables?

amogh-jahagirdar · 2023-06-05T17:15:22Z

@nastra is carrying this forward in #7759, which addresses the pending comments I'll be closing this one just to avoid confusion
Thanks everyone!

github-actions bot added API core labels Jan 11, 2023

amogh-jahagirdar commented Jan 11, 2023

View reviewed changes

jackye1995 reviewed Jan 11, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java Outdated Show resolved Hide resolved

nastra self-requested a review January 13, 2023 14:15

nastra previously requested changes Jan 13, 2023

View reviewed changes

amogh-jahagirdar closed this Jan 13, 2023

amogh-jahagirdar reopened this Jan 13, 2023

rdblue reviewed Jan 30, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/SQLViewRepresentationParser.java Outdated Show resolved Hide resolved

jackye1995 mentioned this pull request Feb 10, 2023

Core: View representation core implementation #6598

Merged

amogh-jahagirdar force-pushed the view-core-impl branch 2 times, most recently from 1be161b to 6d557e9 Compare April 8, 2023 19:54

github-actions bot removed the API label Apr 8, 2023

amogh-jahagirdar force-pushed the view-core-impl branch 3 times, most recently from c617b94 to a526276 Compare April 9, 2023 01:19

jackye1995 reviewed Apr 9, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java Show resolved Hide resolved

jackye1995 reviewed Apr 9, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java Outdated Show resolved Hide resolved

nastra reviewed Apr 11, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java Outdated Show resolved Hide resolved

rdblue reviewed Apr 23, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/view/ViewMetadata.java Outdated Show resolved Hide resolved

rdblue reviewed Apr 23, 2023

View reviewed changes

core/src/test/resources/ValidViewMetadata.json Outdated Show resolved Hide resolved

amogh-jahagirdar mentioned this pull request Apr 24, 2023

API, Core: Move schemaID from ViewRepresentation to ViewVersion and make it required #7421

Merged

amogh-jahagirdar mentioned this pull request May 4, 2023

Spec: Update View spec to reflect that schema is defined at the version level and is required #7485

Merged

amogh-jahagirdar force-pushed the view-core-impl branch 4 times, most recently from fb6ac1f to 508ca82 Compare May 7, 2023 18:30

amogh-jahagirdar requested review from nastra, rdblue, jackye1995 and stevenzwu May 7, 2023 18:33

Core: View metadata implementations

7ea7bfb

Co-authored-by: John Zhuge <[email protected]>

amogh-jahagirdar force-pushed the view-core-impl branch from 508ca82 to 7ea7bfb Compare May 7, 2023 18:34

nastra reviewed May 8, 2023

View reviewed changes

jackye1995 reviewed May 8, 2023

View reviewed changes

rdblue reviewed May 21, 2023

View reviewed changes

stevenzwu reviewed May 25, 2023

View reviewed changes

amogh-jahagirdar closed this Jun 5, 2023

nastra mentioned this pull request Jun 6, 2023

Core: View metadata implementation #7759

Merged

Core: View metadata implementation #6559

Core: View metadata implementation #6559

Conversation

amogh-jahagirdar commented Jan 11, 2023

amogh-jahagirdar Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackye1995 commented Jan 11, 2023

amogh-jahagirdar commented Jan 11, 2023 • edited Loading

nastra left a comment • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar commented Jan 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra Jun 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar commented Jun 5, 2023

amogh-jahagirdar Jan 11, 2023 •

edited

Loading

amogh-jahagirdar Jan 11, 2023 •

edited

Loading

amogh-jahagirdar commented Jan 11, 2023 •

edited

Loading

nastra left a comment •

edited

Loading

nastra Jun 3, 2023 •

edited

Loading