Iceberg View Rest Catalog support #19818

amogh-jahagirdar · 2023-11-19T08:55:05Z

Description

This change integrates Iceberg View support https://iceberg.apache.org/view-spec/ in the REST catalog.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(X ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java

...java/io/trino/plugin/iceberg/catalog/rest/TestIcebergTrinoRestCatalogConnectorSmokeTest.java

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/TestTrinoRestCatalog.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

amogh-jahagirdar · 2023-11-28T20:27:42Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

+                    // Set a best effort view definition
+                    viewDefinition = Optional.of(new ConnectorViewDefinition(
+                            sql, defaultCatalog, defaultNamespace, viewColumns, comment, Optional.empty(), false, null));


I think this will be a key point of discussion but yes best effort means "If we didn't find a Trino dialect, let's use an arbitrary SQL dialect". So here are my thoughts:

Most views are trivial (such as simple projections, aggregations and joins) which share the same semantics across engines.

One of the benefits of having a shared view metadata is being able to access information in other dialects.

Considering 1 and 2, it then seems to be reasonable to simplify the user experience for Trino users, to be able to create views in other systems and then query those same views in Trino without having to do any recreation.

Now, in the current implementation this best effort behavior happens by default. To avoid correctness issues what we can do is make this behavior opt in via a connector property (defaulting it to false to avoid the best effort behavior). That way users who know their domain of queries can take advantage of this benefit of Iceberg view metadata.

On Hive views, those seem to leverage Coral, which I don't think we want to do here. Instead of an IR layer that can be represented as a separate dialect in the future in Iceberg views

amogh-jahagirdar · 2023-11-28T20:44:41Z

Ok so there's still some things I need to look into, here's my list:

1.) Dialect resolution will be a key point of discussion #19818 (comment) -> refer here for the details.

2.) Updating the comment on a column on view should be implemented.

3.) Testing. Right now Trino REST smoke tests use JDBC catalog which doesn't implement ViewCatalog. We pass through some Hadoop HDFS configuration through to the JDBC catalog. For testing views, what I have done currently is a catalog implementation which uses both the JDBC catalog and InMemoryCatalog for views. It's a bit messy, I'll look into options here. Ideally we could just use the InMemoryCatalog but I don't think there's a way to pass in the Hadoop config details we would want.

findepi · 2023-11-29T13:07:09Z

1.) Dialect resolution will be a key point of discussion #19818 (comment) -> refer here for the details.

please see #19818 (comment) for how to unblock this PR.

findepi · 2023-11-29T13:08:25Z

cc @alexjo2144 @findinpath

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java

findinpath · 2023-12-01T09:22:40Z

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java

+        @Override
+        public void createNamespace(Namespace namespace, Map<String, String> metadata)
+        {
+            super.createNamespace(namespace, metadata);


These duplicated calls feel a bit strange.
If the first succeeds and the second call fails , it would be rather misleading from a user perspective to actually see the namespace created for tables, even though the call failed.

findinpath · 2023-12-01T09:25:31Z

plugin/trino-iceberg/pom.xml

@@ -26,7 +26,7 @@
        <air.test.parallel>instances</air.test.parallel>
        <dep.keycloak.version>21.1.2</dep.keycloak.version>
        <!-- Nessie version (matching to Iceberg release) must be bumped along with Iceberg version bump to avoid compatibility issues -->
-        <dep.nessie.version>0.71.0</dep.nessie.version>


Separate commit -> relates probably to iceberg 1.5.x bump (first commit in the PR)

1.5.0 release should be sometime soon (hopefully this month but of course depends). I've been using the snapshot release for development of this feature. I think what most likely would happen is when 1.5.0 is released we'd have a separate PR out for the dependency upgrade and I'd rebase this change on top of that after it gets merged. Let me know what you think @findepi @findinpath , alternatively I could include it as separate commit in this PR

So, this PR can't be merged until 1.5.0 is released right? (It has dependency on some view APIs?)

I sent a mail yesterday in Iceberg mailing list about 1.5.0 release. Lets make the release happen within this month. Then a separate PR to bump Iceberg version and this PR can be merged after that.

But for development we can depend on snapshot version and keep the PR as WIP or Draft.

@ajantha-bhat Yes that's what I mean. After 1.5, we'd most likely have a PR just for the upgrade and then just rebase this change.

@findepi @findinpath Just a heads up, all the dependency updates you see in this PR such as the Iceberg 1.5 update and the Nessie update will be in a separate PR, along with any of the relevant changes that come up as part of the upgrade. https://github.com/trinodb/trino/pull/20308/files is the PR.

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

electrum

This looks good to me overall. I agree with @dain's design comments. Once those and the other comments are addressed, this is good to merge. Thanks for all your work and patience on this.

core/trino-spi/src/main/java/io/trino/spi/StandardErrorCode.java

core/trino-spi/src/main/java/io/trino/spi/connector/UnsupportedViewDialectException.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java

electrum · 2024-05-21T01:55:49Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

+    public static final String ICEBERG_VIEW_RUN_AS_INVOKER = "trino_run_as_invoker";
+    public static final String ICEBERG_VIEW_OWNER = "trino_owner";


That makes sense to me. We also only need the single field trino_run_as_owner that contains the owner. If this is present, then it's a definer view with that owner. If empty, it's an invoker view.

ConnectorViewDefinition has both a boolean and an optional owner for legacy reasons. In the engine, ViewDefinition has a single Optional<Identity> runAsIdentity field.

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

amogh-jahagirdar · 2024-05-21T20:04:02Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

+    default boolean isView(ConnectorSession session, SchemaTableName viewName)
+    {
+        return getView(session, viewName).isPresent();
+    }


The default implementation of connector metadata#getView will preserve the existing behavior for other connectors by just doing getView().isPresent. IcebergMetadata overrides this implementation so we can still return true in the case of multiple dialects.

amogh-jahagirdar · 2024-05-21T20:09:10Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+            if (e.getErrorCode() == ICEBERG_UNSUPPORTED_VIEW_DIALECT.toErrorCode()) {
+                return true;
+            }


The reason for the Iceberg specific error code is so that we can handle multiple dialects properly in the isView logic and distinguish it from other errors. We can just catch this particular code and return true, and other catalogs in the future can just do what the rest catalog does and it should just work. Another benefit of this is that we don't need to plumb isView all the way down into TrinoCatalog and add to another interface.

amogh-jahagirdar · 2024-05-21T20:21:34Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

+    public static final String ICEBERG_VIEW_RUN_AS_INVOKER = "trino_run_as_invoker";
+    public static final String ICEBERG_VIEW_OWNER = "trino_owner";


Great point, we only really need trino_run_as_owner with the actual owner value. Everything else can be inferred from the existence of that field. I've updated.

core/trino-spi/src/main/java/io/trino/spi/connector/UnsupportedViewDialectException.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java

amogh-jahagirdar · 2024-05-21T21:51:20Z

My latest update addressed all the comments @electrum , thanks for the review!

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java

core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java

electrum · 2024-05-21T22:56:59Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergErrorCode.java

@@ -41,6 +41,7 @@ public enum IcebergErrorCode
    ICEBERG_WRITER_CLOSE_ERROR(14, EXTERNAL),
    ICEBERG_MISSING_METADATA(15, EXTERNAL),
    ICEBERG_WRITER_DATA_ERROR(16, EXTERNAL),
+    ICEBERG_UNSUPPORTED_VIEW_DIALECT(17, EXTERNAL)


Nice, I like making this Iceberg specific

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

electrum · 2024-05-21T23:02:52Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

@@ -120,6 +120,9 @@ public abstract class AbstractTrinoCatalog
        implements TrinoCatalog
 {
    public static final String TRINO_CREATED_BY_VALUE = "Trino Iceberg connector";
+    public static final String ICEBERG_VIEW_SQL_DIALECT = "trino";
+    public static final String ICEBERG_VIEW_RUN_AS_OWNER = "trino_run_as_owner";


Should we name the property trino.run_as_owner?

I think that would be better since typically we namespace properties and separate those with a period. Right now, this is the only property, but down the line there can be more in Trino and we'd want to follow the established pattern.

I think we also want to remove the snake case in favor of dashes. That is the pattern that iceberg typically follows for property names. see https://iceberg.apache.org/docs/1.5.0/view-configuration/?h=replace.dro#view-properties

So the final property name would be:

trino.run-as-owner

Sounds good to me

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java

electrum · 2024-05-22T22:47:20Z

Thanks!

mosabua · 2024-05-23T03:49:02Z

Dont we need to document this somehow? Also .. need release notes entry.

amogh-jahagirdar · 2024-05-23T14:46:30Z

Good point @mosabua we certainly should, I'm happy to raise another PR to document this feature.

mosabua · 2024-05-24T16:50:57Z

Great @amogh-jahagirdar - pull me in as reviewer and I can help with merge

This comment was marked as outdated.

Sign in to view

github-actions bot added the iceberg Iceberg connector label Nov 19, 2023

amogh-jahagirdar mentioned this pull request Nov 19, 2023

Support Iceberg Views in TrinoRestCatalog #14120

Closed

findepi reviewed Nov 21, 2023

View reviewed changes

amogh-jahagirdar force-pushed the iceberg-view-support branch from 9d58ff5 to a74d905 Compare November 28, 2023 19:56

This comment was marked as outdated.

Sign in to view

amogh-jahagirdar force-pushed the iceberg-view-support branch from a74d905 to b8a74a1 Compare November 28, 2023 19:56

This comment was marked as outdated.

Sign in to view

amogh-jahagirdar force-pushed the iceberg-view-support branch from b8a74a1 to 2027ad0 Compare November 28, 2023 19:59

This comment was marked as outdated.

Sign in to view

amogh-jahagirdar force-pushed the iceberg-view-support branch from 2027ad0 to cd13bbe Compare November 28, 2023 20:04

This comment was marked as outdated.

Sign in to view

amogh-jahagirdar commented Nov 28, 2023

View reviewed changes

amogh-jahagirdar marked this pull request as ready for review November 28, 2023 20:33

findepi reviewed Nov 29, 2023

View reviewed changes

This was referenced Nov 29, 2023

Core: Add comment property to ViewProperties apache/iceberg#9181

Merged

Iceberg Rest Catalog: Fix bug where listing namespaces was performed but non-existing namespace was provided #19980

Merged

findinpath reviewed Dec 1, 2023

View reviewed changes

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/rest/RestCatalogTestUtils.java Outdated Show resolved Hide resolved

findinpath reviewed Dec 1, 2023

View reviewed changes

...eberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoIcebergRestCatalogFactory.java Outdated Show resolved Hide resolved

findinpath reviewed Dec 1, 2023

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java Outdated Show resolved Hide resolved

This was referenced Jan 12, 2024

Update REST Catalog impl for smoke tests and remove obsolete override for test ajantha-bhat/trino#1

Merged

WIP: Test Upcoming Iceberg and Nessie version bump #20308

Closed

amogh-jahagirdar force-pushed the iceberg-view-support branch from cd13bbe to c9aea99 Compare January 16, 2024 00:36

cla-bot bot added the cla-signed label Jan 16, 2024

github-actions bot added the docs label Jan 16, 2024

amogh-jahagirdar force-pushed the iceberg-view-support branch from 00661a4 to 86fb06e Compare May 3, 2024 03:39

electrum reviewed May 21, 2024

View reviewed changes

amogh-jahagirdar force-pushed the iceberg-view-support branch 4 times, most recently from 8771019 to eaa61be Compare May 21, 2024 19:59

amogh-jahagirdar commented May 21, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java Outdated Show resolved Hide resolved

amogh-jahagirdar force-pushed the iceberg-view-support branch from eaa61be to c703a23 Compare May 21, 2024 20:02

amogh-jahagirdar commented May 21, 2024

View reviewed changes

amogh-jahagirdar force-pushed the iceberg-view-support branch 2 times, most recently from ab7b28f to a7faa6f Compare May 21, 2024 20:20

amogh-jahagirdar commented May 21, 2024

View reviewed changes

amogh-jahagirdar requested a review from electrum May 21, 2024 21:50

electrum approved these changes May 21, 2024

View reviewed changes

amogh-jahagirdar force-pushed the iceberg-view-support branch 3 times, most recently from b1efa04 to 9fceece Compare May 22, 2024 01:21

github-actions bot added the jdbc Relates to Trino JDBC driver label May 22, 2024

Iceberg View Rest Catalog support

406ed5b

amogh-jahagirdar force-pushed the iceberg-view-support branch from 9fceece to 406ed5b Compare May 22, 2024 16:48

electrum merged commit 8d57334 into trinodb:master May 22, 2024
93 of 94 checks passed

github-actions bot added this to the 449 milestone May 22, 2024

ajantha-bhat mentioned this pull request May 23, 2024

[Feature]: Trino Iceberg view support for Nessie catalog projectnessie/nessie#8574

Closed

colebow mentioned this pull request May 29, 2024

Add Trino 449 release notes #22195

Merged

sdasdasd1 mentioned this pull request Jun 21, 2024

[NEW VERSION] Trino 449-450 stackabletech/trino-operator#604

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iceberg View Rest Catalog support #19818

Iceberg View Rest Catalog support #19818

amogh-jahagirdar commented Nov 19, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

amogh-jahagirdar Nov 28, 2023 •

edited

Loading

amogh-jahagirdar commented Nov 28, 2023 •

edited by findepi

Loading

findepi commented Nov 29, 2023

findepi commented Nov 29, 2023

findinpath Dec 1, 2023

findinpath Dec 1, 2023

amogh-jahagirdar Jan 4, 2024 •

edited

Loading

ajantha-bhat Jan 4, 2024

amogh-jahagirdar Jan 4, 2024

amogh-jahagirdar Jan 16, 2024

electrum left a comment

electrum May 21, 2024

amogh-jahagirdar May 21, 2024

amogh-jahagirdar May 21, 2024

amogh-jahagirdar May 21, 2024

amogh-jahagirdar commented May 21, 2024

electrum May 21, 2024

electrum May 21, 2024

amogh-jahagirdar May 21, 2024

electrum May 22, 2024

electrum commented May 22, 2024

mosabua commented May 23, 2024

amogh-jahagirdar commented May 23, 2024

mosabua commented May 24, 2024

		public static final String ICEBERG_VIEW_RUN_AS_INVOKER = "trino_run_as_invoker";
		public static final String ICEBERG_VIEW_OWNER = "trino_owner";

Iceberg View Rest Catalog support #19818

Iceberg View Rest Catalog support #19818

Conversation

amogh-jahagirdar commented Nov 19, 2023

Description

Additional context and related issues

Release notes

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

amogh-jahagirdar Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

amogh-jahagirdar commented Nov 28, 2023 • edited by findepi Loading

findepi commented Nov 29, 2023

findepi commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Jan 4, 2024 • 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

electrum 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

Choose a reason for hiding this comment

amogh-jahagirdar commented May 21, 2024

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

electrum commented May 22, 2024

mosabua commented May 23, 2024

amogh-jahagirdar commented May 23, 2024

mosabua commented May 24, 2024

amogh-jahagirdar Nov 28, 2023 •

edited

Loading

amogh-jahagirdar commented Nov 28, 2023 •

edited by findepi

Loading

amogh-jahagirdar Jan 4, 2024 •

edited

Loading