Nessie: Support views for NessieCatalog by ajantha-bhat · Pull Request #8909 · apache/iceberg

ajantha-bhat · 2023-10-23T14:48:20Z

Extend NessieCatalog with BaseMetastoreViewCatalog.
Move some common code to NessieUtil for reusing between tables and views.
Generalize ViewCatalogTests to reuse for Nessie catalog.

ajantha-bhat · 2023-10-27T05:47:50Z

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

+                    .isInstanceOf(NoSuchTableException.class)
+                    .hasMessageStartingWith("Table does not exist: ns.view"),
+            throwable ->
+                assertThat(throwable)


Nessie throws AlreadyExistsException for a view in this case. Do we need to wrap it up with NoSuchTableException for a table in this case to generalize like other catalogs?

Even Hive view support has same problem
#8907

Altering core test expectation on a PR that adds a specific view implementation does not look right to me.

If core test expectations are sub-optimal perhaps we can adjust them in a separate PR, then rebase the Nessie view PR.

That said, I think the exception thrown in this test case should be the same across all catalog implementations. WDYT?

So far, REST and inmemory catalog follows one pattern (NoSuchTableException) and all other catalogs follows one pattern (AlreadyExistsException).

I tried making Nessie to follow as REST catalog, but it breaks other testcases. I will check more.

replaceTableViaTransactionThatAlreadyExistsAsView
NessieTableOperations.doRefresh() --> throws AlreadyExistsException. But expecting NoSuchViewException.

If I fix it, another test case (createOrReplaceTableViaTransactionThatAlreadyExistsAsView) fails.
Because from the same place doRefresh() we are expecting two different kind of exceptions. I think test case need to be modified instead of unifying code.

Can we follow the in-memory catalog pattern in Nessie?

I actually think that the way Nessie currently handles this error case is correct by saying that a view with the same name already exists when you try to create a table. To achieve the same for REST is actually slightly more difficult. There's also a TODO in the test a few lines above where I wanted to improve the error reporting for these 2 particular cases that were adjusted in this test. That being said, I'm +1 on these 2 changes in the test

this should be addressed by c63be65

@nastra: Awesome. Thanks for fixing it for REST and in-memory catalog. That should help in having a unified behaviour.

ajantha-bhat · 2023-10-27T05:53:40Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

+    // View loaded from catalog will have all the representation as it parses the view metadata
+    // file.
+    SQLViewRepresentation sqlViewRepresentation =
+        (SQLViewRepresentation) metadata.currentVersion().representations().get(0);


Currently Nessie's IcebergView model stores only one dialect and representation. I didn't change Nessie side to hold multiple representation because it was needed for global state. Now that global state is removed in Nessie, only view metadata location is enough. It will have current version id and all the representation for that version.

it seems weird to only store one and not all of the representations

Either Nessie can store all the representations or None I guess. But storing representations is not really required as global state is removed in Nessie, only view metadata location is enough. Lets see what others think.

cc: @dimas-b, @snazy

Should we even bother with storing representations in Nessie at this point? I'd say, until Nessie supports multiple representations, we can just keep metadata location and ignore all representations here. These fields are nullable in Nessie.

True. I mentioned this above.

Either Nessie can store all the representations or None I guess

I think we don't need to store any representation if we don't need those for metadata in Nessie project. So, I am waiting for @snazy's reply.

All metadata should really be taken directly from the view metadata - Nessie should really not store the dialect nor the SQL representation. I don't understand how this related to "global state" - those things are not related to each other at all.

Also the unconditional cast is error prone IMO - similar to assuming that there is only one representation - either of different kinds or multiple SQL representation.

I don't understand how this related to "global state" - those things are not related to each other at all.

IcebergTable model used to only store table metadata location. But then it started storing other info like schema id, snapshot id etc to support global state. The IcebergView model developed was also based on the design of IcebergTable (which was based on global state). Else just View metadata location was enough. Thats how I thought it is designed.

Nessie should really not store the dialect nor the SQL representation

Ok. I will update as per this.

Aah, SqlText is not nullable. So, I cannot construct without it. I will raise a PR at Nessie side to make it Nullable.

https://github.com/projectnessie/nessie/blob/fff9154da6a19cf289c09f2996b75f3cd15e0ca0/api/model/src/main/java/org/projectnessie/model/IcebergView.java#L56

added a dummy sqlText and dummy dialect as we have concluded that we don't have to modify at Nessie side.

ajantha-bhat · 2023-10-27T06:13:43Z

nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibilityForView.java

+import org.projectnessie.model.Branch;
+import org.projectnessie.model.Reference;
+
+public class TestBranchVisibilityForView extends BaseTestIceberg {


Similar to Table's TestBranchVisibility

This test class duplicates the test cases - most are not specific to views.
It seems the view-specific stuff can be merged into TestBranchVisibility.

This test class duplicates the test cases - most are not specific to views.

I will remove TestBranchVisibilityForView

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java

ajantha-bhat · 2023-10-27T06:14:59Z

PR is ready for review.
cc: @nastra, @dimas-b, @snazy, @adutra

nessie/src/main/java/org/apache/iceberg/nessie/UpdateableReference.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

adutra · 2023-10-28T16:05:52Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

-        maybeThrowSpecializedException((NessieReferenceConflictException) ex);
+    } catch (NessieConflictException | NessieNotFoundException | HttpClientException ex) {
+      NessieUtil.handleExceptionsForCommits(ex, client.refName(), failure);
+    } catch (NessieBadRequestException ex) {


Here too, I think this catch block is a code smell, but this one is trickier to solve.

The issue is that in the test called createTableViaTransactionThatAlreadyExistsAsView, we create a transaction based on current hash (let's call it c0), then we commit something outside the transaction (let's call it c1), then we attempt to commit the transaction.

As a result, the transaction/client is trying to commit using the expected hash of c1, but it should really use c0 as the expected commit hash.

There is a property to convey the commit ID: NESSIE_COMMIT_ID_PROPERTY, but it doesn't work when creating things, only when updating.

I think we could solve the problem by storing the current expected hash in a field inside NessieTableOperations, then passing that to the client:

public class NessieTableOperations extends BaseMetastoreTableOperations { ... private Reference reference; ... NessieTableOperations(...) { ... reference = client.getRef().getReference(); } ... protected void doRefresh() { ... String metadataLocation = null; this.reference = client.getRef().getReference(); ... } ... protected void doCommit(TableMetadata base, TableMetadata metadata) { ... client.commitTable(base, metadata, newMetadataLocation, contentId, key, reference); ... } }

I did a quick experiment with the above suggestion and the test threw NessieConflictException as expected, instead of NessieBadRequestException.

@dimas-b what would you suggest in this situation?

In my understanding, commits will be retried on failure for NessieConflictException and it will call doRefresh() for retry which will again use the new reference?

I think the proper way to fix it is to check if any different kind of content exist for the key while committing this key in doCommit() of NessieTableOperation and NessieViewOperation. But that is an extra round trip with server and might degrade commit performance slightly?

Conflicts will not be retried for NessieReferenceConflictException since we throw specialized errors, see here:

iceberg/nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java

Lines 188 to 192 in 89b3a21

if (exception instanceof NessieReferenceConflictException) {

// Throws a specialized exception, if possible

NessieUtil.maybeThrowSpecializedException(

(NessieReferenceConflictException) exception, false);

}

My main point was: we should get rid of this catch (NessieBadRequestException ex) block as, in my opinion, it denotes a code smell.

I think the proper way to fix it is to check if any different kind of content exist for the key while committing this key in doCommit() of NessieTableOperation and NessieViewOperation. But that is an extra round trip with server and might degrade commit performance slightly?

This solution is viable but would imho be less efficient since, as you mentioned, it involves an extra roundtrip.

I personally think that if NessieTableOperation could "memorize" the commit hash of that last time it did a refresh, then reuse it to commit the table changes, that would solve the problem in a better way since it does not involve fetching contents.

That particular testcase is tricky because createTransaction() will call refresh() which already has code to check if any other content exist with the same name. But during commitTransaction() it will not call refresh() so that code is never hit and we need to check again if any new content created with same key concurrently.

So, I have added that logic now instead of catching NessieBadRequestException later on.

I am not sure about memorize commit hash solution. Last time (some other PR) we concluded that we need to be as stateless as possible. Lets see what others think.

I think it's a good idea to record the current commit hash for "new" table/view commits at the time NessieTableOperations is created. At the same time, if the commit is an update, I think we should use the hash from the metadata (basically track it for views too).

Re: NessieBadRequestException, I agree that we should not try to "handle" it. Instead the catalog should be implemented such that NessieBadRequestException does not happen.

ajantha-bhat · 2023-10-30T15:36:03Z

@nastra and @adutra: Can you please take another look? I have addressed the comments.

nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

adutra · 2023-11-02T12:53:03Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

+    Content content = null;
+    try {
+      content =
+          client.getApi().getContent().key(key).reference(client.getReference()).get().get(key);


Checking for the content here imho defeats the purpose of calling doRefresh only when necessary. I think I still prefer to store the hash of the last refresh, then use it for commit. @dimas-b wdyt?

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java

adutra · 2023-11-02T15:00:51Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieViewCatalog.java

+    // Need a fix from Nessie (https://github.com/projectnessie/nessie/issues/7645)
+  }
+
+  // Overriding the below rename view testcases to exclude checking same view metadata after rename.


This seems problematic to me. It would be better to engage with core devs and check whether the metadata is allowed to have extra properties. @nastra wdyt?

At first glance, the assertion seems indeed a bit restrictive:

assertThat(((BaseView) renamed).operations().current()) .usingRecursiveComparison() .ignoringFieldsOfTypes(Schema.class) .isEqualTo(original);

Maybe change the semantics to "contains all the original fields but may also contain some extra ones?"

I think that would be ok to change the semantics to contains all the original fields but may also contain some extra ones (in case a catalog impl decides to add new properties)

Not just that. The original property might modified during rename for Nessie.
Like "nessie.commit.id" is different for original and renamed one.

Shall I ignore the properties aomparision as a whole?

@ajantha-bhat can you please follow-up on this in a separate PR?

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java

nessie/src/test/java/org/apache/iceberg/nessie/TestBranchVisibilityForView.java

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java

nastra · 2023-11-09T08:36:08Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieViewCatalog.java

+    // Need a fix from Nessie (https://github.com/projectnessie/nessie/issues/7645)
+  }
+
+  // Overriding the below rename view testcases to exclude checking same view metadata after rename.


I think that would be ok to change the semantics to contains all the original fields but may also contain some extra ones (in case a catalog impl decides to add new properties)

nastra · 2023-12-08T10:02:11Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

+  }
+
+  /** Lists Iceberg table or view from the given namespace */
+  protected List<TableIdentifier> listContents(Namespace namespace, Content.Type type) {


is there any value in keeping this (and others) protected? I would have thought it should be ok to make this private, since all the other places would then use the table/view-specific methods

In the NessieCatalog itself I have some common code for content which calls these protected classes.

If I doesn't have common code in NessieCatalog, it will be code duplication.
For example see, NessieCatalog.renameContent()

NessieCatalog.table --> NessieCatalog.content --> NessieClient.content this way it is less code duplication.

NessieCatalog.table --> NessieClient.table --> NessieClient.content will cause duplicate lines of code in NessieCatalog

why can't those places just call renameTable() / renameView() directly?

ajantha-bhat · 2023-12-08T10:04:47Z

@nastra: I have addressed the comments. Thanks for the review.

nessie/src/main/java/org/apache/iceberg/nessie/NessieUtil.java

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

ajantha-bhat · 2023-12-11T07:21:40Z

@nastra: Looks like #9012 is causing test failure for this PR after rebase. I tried fixing at Nessie side. But it opens up another issues. Please take a look.

nastra · 2023-12-11T08:23:15Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

    return client
        .withReference(tableReference.getReference(), tableReference.getHash())
-        .dropTable(identifierWithoutTableReference(identifier, tableReference), purge);
+        .dropTable(identifierWithoutTableReference(identifier, tableReference), false);


@ajantha-bhat shouldn't this keep the purge flag?

dimas-b · 2023-12-11T14:01:42Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java

+        fromReference.equalsIgnoreCase(toReference),
+        "Cannot rename %s '%s' on reference '%s' to '%s' on reference '%s':"
+            + " source and target references must be the same.",
+        NessieUtil.contentTypeString(type).toLowerCase(),


Since the expected text is in English we should use .toLowerCase(Locale.ENGLISH)). Using the default locale can have surprising results.... may be not in this particular case, but we ought to ensure correctness in general, I think.

Try this: assertThat("VIEW".toLowerCase(new Locale("TR"))).isEqualTo("view");

While adding it, I did check the Iceberg code.
.toLowerCase() is used in other places also. So, I followed it.

Fixed for new code added by this PR.

We can have GH issue for existing code to track it.

I did a quick scan of the Iceberg codebase for this, and I do a few places where .toLowerCase() may be a concern:

TableIdentifier.toLoweCase() is used only in CachingCatalog internally, which is probably ok, since the lower case strings are not exposed to the outside.

VectorizedSupport seems like it may be a problem, but I do not really know whether the lower case data is exposed and how.

IcebergRecordObjectInspector converts field names to lower case, and that seems to be affected by the locale problem as various case names are accepted as input parameters to its methods.

jQuery code uses a lot of toLowerCase(), but I do not really know how it is supposed to be used.

@nastra : Do you think this is worth opening an issue? To the best of my knowledge this kind of case conversion can be problematic only in German and Turkish locales. The German locale affects only proper German language words (so it is less of a problem), but the Turkish locale can cause English words to be converted in unexpected ways. Does Iceberg support using its libraries in user-defined locales?

it's probably a good idea to open an issue and raise awareness about this

dimas-b

LGTM overall. Sorry for a few late comments.

dimas-b · 2023-12-11T14:29:51Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieIcebergClient.java

+    // We try to drop the content. Simple retry after ref update.
    try {
      commitRetry(
          String.format("Iceberg delete table %s", identifier),


This may be a view now, not just "table" (cf. line 554).

good catch. Induced during rebase.

dimas-b · 2023-12-11T14:42:33Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

-        maybeThrowSpecializedException((NessieReferenceConflictException) ex);
+    } catch (NessieConflictException | NessieNotFoundException | HttpClientException ex) {
+      if (ex instanceof NessieConflictException || ex instanceof NessieNotFoundException) {
+        failure = true;


I think it might be preferable to delegate determining the failure flag to NessieUtil.handleExceptionsForCommits. This logic should be the same between tables and views.

Perhaps handleExceptionsForCommits could return an object with both the flag and the exception to be re-thrown. WDYT?

That will be more complicated I guess.

I have simplified a bit now. Please take a look

dimas-b · 2023-12-11T14:44:11Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

+      NessieUtil.handleExceptionsForCommits(ex, client.refName(), Content.Type.ICEBERG_TABLE)
+          .ifPresent(
+              exception -> {
+                throw exception;


If I follow the logic correctly, handleExceptionsForCommits will always return something for the exception types caught in this case, so ifPresent() is really confusing. Could the code be refactored to avoid unnecessary conditional execution?

+1 on improving exception handling in the nessie code as I've raised this already before, because it's difficult to read & understand when certain things are thrown.
However, I would suggest to do this as an immediate follow-up after this PR is merged, as otherwise this makes it more difficult to review the changes being introduced for Views

I'm fine with a follow-up.

Slightly improved now ;)

dimas-b · 2023-12-11T14:50:08Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieViewOperations.java

+      throw NessieUtil.handleBadRequestForCommit(client, key, Content.Type.ICEBERG_VIEW).orElse(ex);
+    } finally {
+      if (failure) {
+        io().deleteFile(newMetadataLocation);


This code appears to be shared with NessieTableOperations. Could we refactor it to have the exception/failure/cleanup logic in one place?

I tried before.
But one works on client.commitView and one is client.commitTable. So, it cannot be extracted to common code.
I did also try accepting a generic Runnable to execute. But it cannot throw the required exception. Hence, it has to be duplicated.

We could define our own functional interface with declared exceptions.

nessie/src/test/java/org/apache/iceberg/nessie/BaseTestIceberg.java

dimas-b · 2023-12-11T14:54:08Z

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieViewCatalog.java

+
+  private void resetData() throws NessieConflictException, NessieNotFoundException {
+    Branch defaultBranch = api.getDefaultBranch();
+    for (Reference r : api.getAllReferences().get().getReferences()) {


tangential: This code is repeated in many downstream tests... Perhaps we could make a helper utility in Nessie OSS 🤔

I don't see the advantage of adding at Nessie side. But it can be added as Util for the test classes. But maybe as a follow up PR.

fair enough.

dimas-b · 2023-12-11T15:32:34Z

nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java

+      NessieUtil.handleExceptionsForCommits(ex, client.refName(), Content.Type.ICEBERG_TABLE)
+          .ifPresent(
+              exception -> {
+                throw exception;


I'm fine with a follow-up.

ajantha-bhat marked this pull request as draft October 23, 2023 14:48

github-actions bot added core NESSIE labels Oct 23, 2023

ajantha-bhat force-pushed the nessie_view branch from 66ea0be to 89b3a21 Compare October 26, 2023 16:07

ajantha-bhat commented Oct 27, 2023

View reviewed changes

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieView.java Show resolved Hide resolved

ajantha-bhat marked this pull request as ready for review October 27, 2023 06:14