Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Glue catalog in Iceberg connector #10845

Merged
merged 9 commits into from
Mar 15, 2022

Conversation

alexjo2144
Copy link
Member

@alexjo2144 alexjo2144 commented Jan 28, 2022

Authored by @jackye1995, based off of #10151

I still need to address the testing and figure out what is possible to run on the Trino automation.

@findepi
Copy link
Member

findepi commented Jan 28, 2022

@alexjo2144 can you please squash commits before re-review? it's been a while, so i need to start reading anew anyway.

@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch from 250a301 to ae22437 Compare January 28, 2022 20:20
@alexjo2144
Copy link
Member Author

Squashed. I left a copy of the history on a separate branch in case it's useful to see what I changed from Jack's initial PR https://github.com/alexjo2144/trino/tree/alexjo/glue-iceberg-catalog-full-history

@alexjo2144
Copy link
Member Author

@findepi I took a stab at adapted Jack's TestIcebergGlueConnectorSmokeTest to work on CI. PTAL

@alexjo2144
Copy link
Member Author

Pushed another commit with some more Catalog tests, and one which should fix #10902

@alexjo2144
Copy link
Member Author

I am still working on the inverse problem, where reading the Iceberg Glue information schema fails if Hive tables are present. I'll try to have that fix and a test up later today.

@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch 2 times, most recently from 188868b to 9196e0f Compare February 2, 2022 21:56
@alexjo2144
Copy link
Member Author

I am still working on the inverse problem, where reading the Iceberg Glue information schema fails if Hive tables are present. I'll try to have that fix and a test up later today.

Just pushed an update for that in the last commit along with some tests for that case. The fixups were getting a little out of hand so I did squash them.

@alexjo2144
Copy link
Member Author

One other thing I noticed while I was writing these tests though. SHOW TABLES FROM iceberg.information_schema fails on Glue but passes on the Hive catalog.

@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch 2 times, most recently from 5af448e to 3d88427 Compare February 8, 2022 19:35
@alexjo2144
Copy link
Member Author

@findepi I pushed an update with a fix for table redirections, and some tests with a Glue metastore that has Hive and Iceberg tables in the same namespace.

}

@Test
public void testDefaultLocation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in BaseTestTrinoCatalog?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests the hive.metastore.glue.default-warehouse-dir config, which is Glue specific

@findepi
Copy link
Member

findepi commented Feb 15, 2022

@alexjo2144 can you please extract Handle missing StorageDescriptor in Hive Glue tables to a separate PR?
once it's merged, i will update #10881

doing otherwise would cause breakage for the master CI runs, right?

@RameshByndoor
Copy link
Member

@alexjo2144 Thanks, we are waiting to see this feature released. Can you please help with when is this going to be available?

@RameshByndoor
Copy link
Member

Tried to use this patch with the latest 371 version. But failed with the following error, the same can be seen in mvn tests as well.

1) [Guice/JitDisabled]: Explicit bindings are required and HiveMetastoreFactory annotated with @RawHiveMetastoreFactory() is not explicitly bound.
  at DecoratedHiveMetastoreModule.createHiveMetastore(DecoratedHiveMetastoreModule.java:70)
      \_ for 1st parameter metastoreFactory
  at DecoratedHiveMetastoreModule.createHiveMetastore(DecoratedHiveMetastoreModule.java:70)
      \_ installed by: IcebergCatalogModule -> DecoratedHiveMetastoreModule

Any workaround?

@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch from 3d88427 to 682f3c7 Compare February 28, 2022 20:50
@alexjo2144
Copy link
Member Author

Pushed a fixup with a small test fix related to @electrum 's changes

@findepi findepi force-pushed the alexjo/glue-iceberg-catalog branch from d78ab73 to 3a12e01 Compare March 14, 2022 13:41
@findepi
Copy link
Member

findepi commented Mar 14, 2022

(squashed)

@mosabua
Copy link
Member

mosabua commented Mar 14, 2022

From a users perspective (and as mentioned in the linked documentation review) the choice of "catalog type" is not really logical and also inconsistent with the Hive connector (and maybe others). That configuration only sets the metastore and it therefore should be called as such. iceberg.metastore .. and NOT catalog.type.

See https://trino.io/docs/current/connector/hive.html#metastore-configuration-properties

And then we can also namespace the specific properties nicely..

iceberg.metastore.glue.* and maybe iceberg.metastore.thrift (or hms or hive)

Wdyt @findepi @electrum ?

And btw.. sorry for chiming in late .. I just saw the doc PR now..

@findepi
Copy link
Member

findepi commented Mar 14, 2022

@mosabua please see #9577 .

@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch from 3a12e01 to cf36012 Compare March 14, 2022 17:39
@alexjo2144 alexjo2144 force-pushed the alexjo/glue-iceberg-catalog branch from cf36012 to 30ab6ac Compare March 14, 2022 21:04
@mosabua
Copy link
Member

mosabua commented Mar 15, 2022

Hm .. very interesting @findepi .. thanks for the link. Its a bit unfortunate that catalog type is overloaded as a term in Iceberg .. but I guess that is out of our control...

@alexjo2144
Copy link
Member Author

Its a bit unfortunate that catalog type is overloaded as a term in Iceberg

Agreed

@findepi @electrum builds are green! Can we go ahead and merge this in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants