Core, API: View Version implementation #6861

amogh-jahagirdar · 2023-02-16T16:26:46Z

View Version parser implementation

Co-authored-by: John Zhuge jzhuge@apache.org

CC @jzhuge @rdblue @nastra @danielcweeks @jackye1995 Let me know your thoughts!

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

nastra

just a few small things to address, but other than that it LGTM

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

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

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

amogh-jahagirdar · 2023-02-22T00:46:48Z

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

@@ -43,8 +47,7 @@ public interface ViewVersion {
  long timestampMillis();

  /**
-   * Return the version summary such as the name of the operation that created that version of the
-   * view
+   * Return a string map of summary data for the operation that produced this view version.


In the data model itself, I think we should follow what Snapshot does where summary returns all the fields other than the operation itself. We have a separate operation method below for the operation. When we serialize the version, the operation will get put in the summary of the version. Let me know if this makes sense @nastra @jackye1995 @rdblue

I think we can avoid having a change of spec here, the information in summary are just FYI purpose, I don't think we need to assign a semantic meaning to operations now, we can always add later if it becomes necessary. But for views I don't see anything other than CREATE and REPLACE, but it can also be identified directly through the version ID, ID=1 means CREATE, everything else are REPLACE.

Sorry for the confusion here, there's no change in spec here, it's a change in the data model layer but since there was no implementation until now we have some flexibility to change if desired.

We'll still serialize the operation in the summary map when writing the metadata file, so the spec is maintained.

When we deserialize into the in memory representation, the summary map will not contain operation, and instead a user would use the operation() API. It's a similar model to Snapshot and SnapshotParser

Let me know if that makes sense, or if there's doubts on the API itself after this change! cc: @rdblue @nastra @danielcweeks @jzhuge

I think the argument also applies to the data model, because for Snapshot I am interested in what is the operation that produces the snapshot, because there are different types for data write. But for view, I am not sure how useful that information is, it's always just create or replace of the metadata definition. So I don't know if we should add this layer of complication.

Discussed offline with @jackye1995 instead of restricting the type of operation up front, we can keep it open. It looks like Snapshot summary also does has the same behavior. There's a set of recognized operations by the Iceberg library like append, replace etc but those aren't strictly validated when reading the metadata. So that's a similar model we can follow here. We also don't need an enum following this logic, it can just be defined Strings (which we can define separately when we get to the catalog implementation for createView replaceView etc. Also we can always add an operation API if it becomes to awkward to get the operation from the summary but it's not needed at the moment.

Discussed offline with @jackye1995 we were talking on two different issues here.

1.) The first is on strict validation of the operation, we concluded that's probably not a desired behavior, and that's the behavior of snapshot currently.

2.) The second is on the API and serialization of operation itself. Operation will be serialized in the summary but when it's read we deserialize it into a separate field in memory. This is the same as what happens with Snapshot and I think it makes sense to have that same behavior and have a separate API for operation. Updated the PR to reflect this.

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

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

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

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

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

nastra

still LGTM, thanks @amogh-jahagirdar

jackye1995

looks good to me!

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

jackye1995 · 2023-03-21T15:55:01Z

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

+    generator.writeNumberField(VERSION_ID, version.versionId());
+    generator.writeNumberField(TIMESTAMP_MS, version.timestampMillis());
+    generator.writeObjectFieldStart(SUMMARY);
+    for (Map.Entry<String, String> summaryEntry : version.summary().entrySet()) {


shouldn't we also write OPERATION here?

The summary should be guaranteed to have operation at the time of writing the metadata (should already be an entry in the map).

updated, see #6861 (comment) for the discussion on this

jackye1995

looks good to me!

Co-authored-by: John Zhuge <jzhuge@apache.org>

jackye1995 · 2023-03-22T00:29:24Z

Thanks for all the changes, let me know when you have the metadata parser PR!

github-actions bot added API core labels Feb 16, 2023

amogh-jahagirdar commented Feb 16, 2023

View reviewed changes

api/src/main/java/org/apache/iceberg/view/ViewVersion.java Outdated Show resolved Hide resolved

jackye1995 self-requested a review February 16, 2023 18:17

nastra reviewed Feb 17, 2023

View reviewed changes

amogh-jahagirdar commented Feb 21, 2023

View reviewed changes

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

amogh-jahagirdar force-pushed the view-version branch 2 times, most recently from 03783c8 to baf9be2 Compare February 22, 2023 00:49

amogh-jahagirdar commented Feb 22, 2023

View reviewed changes

nastra approved these changes Feb 22, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the view-version branch from baf9be2 to 98f8784 Compare March 8, 2023 15:51

amogh-jahagirdar commented Mar 8, 2023

View reviewed changes

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

amogh-jahagirdar force-pushed the view-version branch from 98f8784 to 616c1f0 Compare March 9, 2023 04:42

jackye1995 reviewed Mar 9, 2023

View reviewed changes

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

jackye1995 reviewed Mar 9, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/view/TestViewVersionParser.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the view-version branch 3 times, most recently from 2170c9f to c6dc13c Compare March 10, 2023 02:42

nastra approved these changes Mar 10, 2023

View reviewed changes

jackye1995 approved these changes Mar 10, 2023

View reviewed changes

jackye1995 reviewed Mar 21, 2023

View reviewed changes

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

jackye1995 reviewed Mar 21, 2023

View reviewed changes

amogh-jahagirdar force-pushed the view-version branch 3 times, most recently from e4d60b6 to 35562fc Compare March 21, 2023 20:44

jackye1995 approved these changes Mar 21, 2023

View reviewed changes

amogh-jahagirdar force-pushed the view-version branch 2 times, most recently from 3b43bf8 to d6968e7 Compare March 21, 2023 21:11

Core, API: View Version implementation

59feecc

Co-authored-by: John Zhuge <jzhuge@apache.org>

amogh-jahagirdar force-pushed the view-version branch from d6968e7 to 59feecc Compare March 21, 2023 22:12

jackye1995 merged commit b099a33 into apache:master Mar 22, 2023

nastra added this to Done in View support Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core, API: View Version implementation #6861

Core, API: View Version implementation #6861

amogh-jahagirdar commented Feb 16, 2023 •

edited

nastra left a comment

amogh-jahagirdar Feb 22, 2023

jackye1995 Feb 28, 2023

amogh-jahagirdar Mar 8, 2023 •

edited

jackye1995 Mar 8, 2023

amogh-jahagirdar Mar 9, 2023

amogh-jahagirdar Mar 21, 2023

nastra left a comment

jackye1995 left a comment

jackye1995 Mar 21, 2023

amogh-jahagirdar Mar 21, 2023

amogh-jahagirdar Mar 21, 2023

jackye1995 left a comment

jackye1995 commented Mar 22, 2023

Core, API: View Version implementation #6861

Core, API: View Version implementation #6861

Conversation

amogh-jahagirdar commented Feb 16, 2023 • edited

nastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Mar 8, 2023 • edited

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 left a comment

Choose a reason for hiding this comment

jackye1995 left a 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

jackye1995 left a comment

Choose a reason for hiding this comment

jackye1995 commented Mar 22, 2023

amogh-jahagirdar commented Feb 16, 2023 •

edited

amogh-jahagirdar Mar 8, 2023 •

edited