Add Iceberg RESTSessionCatalog Implementation

[danielcweeks]

Description

This PR adds and the REST Catalog implementation for Iceberg.

Is this change a fix, improvement, new feature, refactoring, or other?

New Feature / Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector: Iceberg

How would you describe this change to a non-technical end user or system administrator?

This allows Trino to access any data source that supports the Iceberg REST Catalog spec.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
() Release notes entries required with the following suggested text:

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

[ebyhr]

Please add connector and product test. You can refer to #11772 as the example.

[alexjo2144]

Overall, looks pretty good. Mostly style/nit pick comments

[alexjo2144]

Not having View or MV support is pretty limiting. Is that a restriction from the backend, or can we implement them as follow up?

[danielcweeks]

Not having View or MV support is pretty limiting. Is that a restriction from the backend, or can we implement them as follow up?

We definitely want to add view support as a follow-up. However, the iceberg view spec is still in review, so we should wait until that is finalized.

[ebyhr]

Lett initial comments. Is it possible to add a test for iceberg.rest.token and iceberg.rest.credential?

Also, could you update documentation iceberg.rst?

[danielcweeks]

Lett initial comments. Is it possible to add a test for iceberg.rest.token and iceberg.rest.credential?

@ebyhr It may be possible to add tests to validate that the token and credential values are passed through, but would require a lot of additional infrastructure to support actually testing the OAuth2 flows. The logic for all of that is already handled by the underlying Iceberg library and there are extensive tests to validate the flow behaviors.

[electrum]

Generally looks good

[electrum]

Why do we need this? If we're going to enable preview features, seems like we should make that decision for all of Trino and set it at the top level.

[nastra]

I can't recall whether we added that or whether this was a leftover after a rebase. I've removed it

[electrum]

Agreed, I like the iceberg.rest-catalog prefix

[electrum]

Is this required? Can we file an issue in Iceberg to make this not depend on Hadoop? We're trying to eliminate Hadoop as a required dependency for Iceberg and other connectors.

[rdblue]

I think we were asked to remove the FileIO implementations other than HadoopFileIO, which requires a Configuration. Should we add the iceberg-aws dependency back and make this optional?

[nastra]

This is required unfortunately for ResolvingFileIO here

[danielcweeks]

Just to clarify the other responses, the REST Catalog does not have any runtime Hadoop dependencies. However, Trino performs IO operations using TrinoFileSystem (via HadoopFileIO), which does requires a Hadoop configuration. I don't think we want to add scope to this PR by trying to change that path, but whenever alternatives are available, this won't be a blocker.

[electrum]

Do we need a new test group? What is different between ICEBERG and ICEBERG_REST?

[nastra]

we mainly introduced a new test group that allows running only a subset of tests, otherwise we'd have to make more changes to existing product tests to make them work across different environments.
In the long run (and in a follow-up PR) however I think we could aim for removing that particular test group if that's desired.

[electrum]

Is source generic in the REST catalog? How might it be used?

While it might be be useful for informational or auditing purposes, we need to ensure that it won't be used for access control, returning different information depending on the value, or that caching results with different source values is ok. Same concerns as the user.

[electrum]

Passing the user is impersonation, which has various implications. We need a config for this which is disabled by default. When disabled, we need a config to specify the shared system user to pass here.

If we're going to support impersonation, I think we need some guarantees in the REST catalog specification that will not prevent caching:

• The user is only used for access control, not auditing.
• Name mappings and table data are the same for all users. In other words, loadTable() will return the same information for all users, or will be denied. It won't return different information depending on the user.

[jebnix]

I think that it will be good to add some explanation about how the REST Catalog works. 3-4 sentences would help people very much with adopting this. General explanations of the implications of using this would also be a good addition (e.g. does the usage of this enables us to deploy Trino with Iceberg connector without Hive Metastore?).

Otherwise, seems awesome

[code-magician323]

@jebnix I think the current docs are OK. @danielcweeks correct me if I'm wrong but there is no need for Hive Metastore when configuring Iceberg with the REST Catalog, but it is required to use some Catalog behind the scenes (e.g. Nessie / JDBC)

[danielcweeks]

@jebnix I think the current docs are OK. @danielcweeks correct me if I'm wrong but there is no need for Hive Metastore when configuring Iceberg with the REST Catalog, but it is required to use some Catalog behind the scenes (e.g. Nessie / JDBC)

That's correct. There's no dependency on Hive, but you need a server side implementation of the REST spec, which can just be a proxied version of one of the existing Iceberg catalog implementations.

[danielcweeks]

@electrum I've updated to add the configuration for session info which defaults to NONE and there is a PR to clarify the REST behavior. Please take another look when you can.

[jebnix]

@danielcweeks @nastra Do you think this can get merged soon? Trino still cannot use Iceberg without a Hive Metastore. That's the most important feature of the Iceberg Connector by far.

[nastra]

I believe nothing is left to be done from our side, it is mainly waiting for a final approval

[code-magician323]

@bitsondatadev Why doesn't this get merged?

[bitsondatadev]

@bitsondatadev Why doesn't this get merged?

I'm gonna tag in @electrum and @findepi here. I'm seeing activity in general start to slow down for development as we approach the holidays so I wouldn't hold your breathe for too much happening until after new years.

Just like Santa 🎅 I'm making a list but my list contains PRs I'll be checking twice when I get back from break.

[bitsondatadev]

@bitsondatadev Why doesn't this get merged?

I'm gonna tag in @electrum and @findepi here. I'm seeing activity in general start to slow down for development as we approach the holidays so I wouldn't hold your breathe for too much happening until after new years.

Just like Santa 🎅 I'm making a list but my list contains PRs I'll be checking twice when I get back from break.

I stand corrected! Happy holidays!