-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add support for the custom hive catalog to support Hive3 #21502
Conversation
4dd8066
to
141b9e7
Compare
cc @samssh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skimmed and first pass
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHttpMetastoreConfig.java
Outdated
Show resolved
Hide resolved
@osscm now that you have a "ready to review" PR could you please update the description to contain the business case & concise notes including the implementation strategy contained in the code of this PR? |
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osscm TestThriftHttpMetastoreClient is failing.
You need to fix https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java#L60 as something like
if (databaseName.equals("@hive#testDbName")) {
return new Database("testDbName", "testOwner", "testLocation", Map.of("key", "value"));
}
and also TestingThriftHttpMetastoreServer to have the new method called
case "getAllDatabases", "getDatabases" -> delegate.getAllDatabases();
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
@electrum can you enable test with secrets in this PR? |
thanks @findinpath cc @anusudarsan |
/test-with-secrets sha=e7556cb2cb2db99b03236409fcc1d1186badf430 |
The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/8748163326 |
@osscm re: tests can we extend
I guess you will also need to create a
let me know if you hit any issues with this or need help. @findinpath @electrum is extending |
fyi, will work on the pending comments today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % open comments
/test-with-secrets sha=cf153475e20161039b1e99815c414b2bf2bfafb5 |
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8977088500 |
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestHiveMetastoreCatalogs.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestHiveMetastoreCatalogs.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestHiveMetastoreCatalogs.java
Outdated
Show resolved
Hide resolved
plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/TestHiveMetastoreCatalogs.java
Outdated
Show resolved
Hide resolved
...-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestThriftHttpMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
Outdated
Show resolved
Hide resolved
...ive/src/test/java/io/trino/plugin/hive/TestHiveConnectorCustomCatalogConnectorSmokeTest.java
Outdated
Show resolved
Hide resolved
sorry, got engaged in other work, will try to take care of remaining comments in next a few days. |
c62f95e
to
f0b0a7f
Compare
@osscm I pushed some changes instead of you. |
@osscm Why did you revert my change? |
did I revert? |
@osscm I restored to the status before you reverted my change. Please add a fixup commit for additional changes. |
Thanks @ebyhr! I think, already it has all the changes in! so dont need to do any changes. two test cases are failing, checking why
this one I am not sure, was not able to run in local, we can try to run build again?
|
@osscm the CI looks good now |
@anusudarsan, so good to merge now? :) |
Description
Currently Trino is constrained to utilize the default hive catalog (referred to as
"hive"
) when interfacing with the hive metastore. Consequently, users are confined to employing only a two-level hierarchy (schema.table
) within a Trino catalog/connector. In contrast, Hive thrift allows for custom hive catalog names as the parent of the schema, enabling the utilization of a three-level hierarchy such ashive-catalog.schema.table
.Hive Thrift API supports custom catalog since Hive3
Therefore, at high level, this PR has the following changes
StaticMetastoreConfig
,StaticTokenAwareHttpMetastoreClientFactory
,StaticTokenAwareMetastoreClientFactory
,ThriftMetastoreClientFactory
, 'DefaultThriftMetastoreClientFactory',HttpThriftMetastoreClientFactory
ThriftHiveMetastoreClient
"hive"
will be the default catalog name.Thanks to @dain @electrum @hashhar @findinpath @anusudarsan @samssh @mosabua
Additional context and related issues
Above approach was discussed at a few places, so adopted in the PR as well.
one
two
@samssh please let us know, if anything is missing or need to take care of.
Fixes #10287
co-author : @samssh, he has worked on the similar change.
Release notes
(x) Release notes are required, with the following suggested text: