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.

Sign up for GitHub

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

Jump to bottom

Hide Delta Lake tables #5430

Merged
merged 2 commits into from
Oct 9, 2020
Merged

Hide Delta Lake tables #5430

merged 2 commits into from
Oct 9, 2020

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 6, 2020

Hive connector cannot read from Delta Lake tables and some users prefer
not to see such tables at all.

@cla-bot cla-bot bot added the cla-signed label Oct 6, 2020
@mosabua mosabua added the needs-docs This pull request requires changes to the documentation label Oct 6, 2020
losipiuk
losipiuk approved these changes Oct 6, 2020
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastoreConfig.java Outdated Show resolved Hide resolved
presto-hive-hadoop2/src/test/java/io/prestosql/plugin/hive/TestHive.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConfig.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/alluxio/AlluxioHiveMetastore.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java
.map(Path::getName)
return listAllTables(databaseName).stream()
.filter(hideDeltaLakeTables
? Predicate.not(ImmutableSet.copyOf(getTablesWithParameter(databaseName, SPARK_TABLE_PROVIDER_KEY, DELTA_LAKE_PROVIDER))::contains)
Copy link
Member

Choose a reason for hiding this comment

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

static import.
Though I personally would prefer not using Predicate.not at all and just extend the lambda, and use !

Copy link
Member Author

Choose a reason for hiding this comment

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

static import what?

Though I personally would prefer not using Predicate.not at all and just extend the lambda, and use !

the not is necessary to make ImmutableSet.copyOf(....) computed once

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Oct 6, 2020

We need to document this property. Maybe even a separate section about Delta tables somewhere.

@findepi findepi mentioned this pull request Oct 6, 2020
Merged
@findepi
Copy link
Member Author

findepi commented Oct 6, 2020

I'll move the config property from HiveConfig to MetastoreConfig to please iceberg.

electrum
electrum requested changes Oct 6, 2020
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastoreConfig.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileMetastoreModule.java Outdated Show resolved Hide resolved
presto-hive-hadoop2/src/test/java/io/prestosql/plugin/hive/TestHiveAlluxioMetastore.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveTableProperties.java Outdated Show resolved Hide resolved
@findepi findepi mentioned this pull request Oct 6, 2020
Merged
@phd3 phd3 mentioned this pull request Oct 6, 2020
Closed
@findepi findepi requested a review from electrum October 7, 2020 09:19
@findepi findepi force-pushed the findepi/hide-delta-tables branch 2 times, most recently from f30fbe4 to 8799cbb Compare October 7, 2020 09:28
@findepi
Copy link
Member Author

findepi commented Oct 7, 2020

AC

@findepi
Copy link
Member Author

findepi commented Oct 8, 2020

AC

@findepi findepi requested a review from electrum October 8, 2020 10:34
electrum
electrum approved these changes Oct 8, 2020
View reviewed changes
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
}
return ImmutableList.of(tableName);
return optionalTable
Copy link
Member

Choose a reason for hiding this comment

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

This could be done inline, no need for the variable

return metastore.getTable(...)
    .filter(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted the catch(HiveViewNotSupportedException to cover as little as required

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreConfig.java Outdated
return hideDeltaLakeTables;
}

@Config("hive.hide-delta-tables")
Copy link
Member

Choose a reason for hiding this comment

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

Let's be explicit and call this hive.hide-delta-lake-tables

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch

@findepi
Copy link
Member Author

findepi commented Oct 8, 2020

AC

@dain
Copy link
Member

dain commented Oct 8, 2020

I was hoping the config would generic like hive.hide-unsupported-tables. I’m afraid we will have one of these for every storage handler added to Hive…. so Delta, Iceberg, HBase, Carbon Data, Hudi, Cassandra, MongoDB, Elastic Search, etc..

@findepi
Copy link
Member Author

findepi commented Oct 8, 2020

@dain Cool idea!

There are currently many unsupported table types (eg HBase, JdbcStorageHandler), and having a generic-looking config, which -- as it stands today -- filters out Delta Lake tables only would not be easy to understand.

Still, we can easily rename the config when we expand the functionality. Would you mind filing an issue for that?

@findepi
Hide Delta Lake tables
1f4681b 
Hive connector cannot read from Delta Lake tables and some users prefer
not to see such tables at all.
@findepi findepi merged commit 6108b9d into master Oct 9, 2020
@findepi findepi deleted the findepi/hide-delta-tables branch October 9, 2020 07:35
@findepi findepi mentioned this pull request Oct 9, 2020
Closed
10 tasks
@findepi findepi added this to the 344 milestone Oct 9, 2020
@findepi
Copy link
Member Author

findepi commented Oct 9, 2020

@mosabua can you please follow up with docs?
the hive.hide-delta-lake-tables config is always available, but is currently supported only for Glue metastore (and File metastore, which is purposefully not documented).
Not sure how this should be presented in docs.

@dmitryfill
Copy link

@findepi - Can Iceberg tables be hidden from Hive connector as well?

@findepi
Copy link
Member Author

findepi commented Oct 12, 2020

@dmitryfill good idea!
see the ticket #5482 for making the feature more broadly applicable
do you want to take a stab at this?

also, please be aware there is some work to allow a redirect from hive to iceberg, see #4704

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

@losipiuk losipiuk losipiuk approved these changes

@electrum electrum electrum approved these changes

Assignees
No one assigned
Labels
cla-signed needs-docs This pull request requires changes to the documentation
Milestone
344
Development

Successfully merging this pull request may close these issues.
6 participants
@findepi @mosabua @dain @dmitryfill @electrum @losipiuk