Enforce hive.translate-hive-views propertry #5636

phd3 · 2020-10-21T17:52:38Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

laurachenyu · 2020-10-21T18:20:30Z

Should we throw exception here or just return Optional.empty() like before? I am doing following:

    {
        if (!filterSchema(viewName.getSchemaName())) {
            return Optional.empty();
        }
        return metastore.getTable(new HiveIdentity(session), viewName.getSchemaName(), viewName.getTableName())
                .filter(ViewReaderUtil::canDecodeView)
                .flatMap(view -> {
                    if (translateHiveViews || isPrestoView(view)) {
                        ConnectorViewDefinition definition = createViewReader(metastore, new HiveIdentity(session), view, typeManager)
                                .decodeViewData(view.getViewOriginalText().get(), view, catalogName);
                        // use owner from table metadata if it exists
                        if (view.getOwner() != null && !definition.isRunAsInvoker()) {
                            definition = new ConnectorViewDefinition(
                                    definition.getOriginalSql(),
                                    definition.getCatalog(),
                                    definition.getSchema(),
                                    definition.getColumns(),
                                    definition.getComment(),
                                    Optional.of(view.getOwner()),
                                    false);
                        }
                        return Optional.of(definition);
                    }
                    return Optional.empty();
                });
    }```

Also, ThriftHiveMetastore:getTable() also used tranlativeHiveViews and we can recover it as well:

                    if (!translateHiveViews
                            && table.getTableType().equals(TableType.VIRTUAL_VIEW.name())
                            && !"true".equals(table.getParameters().get(PRESTO_VIEW_FLAG))) {
                        throw new HiveViewNotSupportedException(new SchemaTableName(databaseName, tableName));
                    }

phd3 · 2020-10-22T03:31:34Z

@laurachenyu your suggestion also looks good. One difference though is that previously the isPrestoView api used hive's Table object, but now we use Presto's object, so that logic would be kind of duplicated. I'm fine with either approaches, @kokosing wdyt?

kokosing

I think we should return Optional.empty() when hive view is not supported. Otherwise it does make sense to have a method that returns Optional when it fails in case it cannot return a value.

kokosing · 2020-10-22T07:43:06Z

presto-hive/src/test/java/io/prestosql/plugin/hive/AbstractTestHive.java

+            ConnectorMetadata metadata = transaction.getMetadata();
+            assertThatThrownBy(() -> metadata.getView(session, new SchemaTableName(database, tableName)))
+                    .isInstanceOf(HiveViewNotSupportedException.class)
+                    .hasMessageContaining("Hive views are not supported");


It would be nice add a test where changing config property would actually return a view here.

We have product tests for that, so if adding a test here is problematic then maybe a comment would be enough.

phd3 · 2020-10-22T17:35:24Z

Updated based on the suggestions, However, not sure if this is the best way to test. currently, it verifies the logic for ThriftHiveMetastore and FileHiveMetastore through TestHive and TestHiveFileMetastore. For GlueMetastore, it seems that we haven't tested the support for the hive view translation so far, so I think that can be done in a separate effort.

In general, I feel that the testing related to view translation "integration" can be improved using one of the approaches below:

approach-1: (similar to current updated PR)

integration logic can be in different HiveMetastore implementations.
need to test them through metastore-specific tests, since they've different codepaths (e.g. a test in AbstractTestHive). If we want to test both the cases (enabled/disabled translation), it requires some amount of refactoring of AbstractTestHive.

approach-2:

Keep all integration related logic in HiveMetadata class
Test the cases for view-translation enabled and disabled using FileHiveMetastore.

approach-3:

make the translation enabling switch a session property and test along with product tests by enabling/disabling it.

I feel approach-2 and approach-3 might be easier, but approach-1 is more exhaustive. however, need to keep in mind that we don't overfit the code-implementation on the ease of testing :)

electrum · 2020-10-22T22:22:47Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

@@ -314,7 +316,15 @@ else if (table.getTableType().equals(EXTERNAL_TABLE.name())) {
    @Override
    public synchronized Optional<Table> getTable(HiveIdentity identity, String databaseName, String tableName)
    {
-        return getTable(databaseName, tableName);
+        Optional<Table> optionalTable = getTable(databaseName, tableName);


We shouldn't need any view specific logic in the metastore, as the metastore's job is to simply store/retrieve the table metadata. This specific check seems to be a sanity check that the file metastore doesn't contain a Hive view, but that should be impossible since the code is only used by Presto.

got it, agreed. ThriftHiveMetastore::getAllViews also involves usage of view specific config, may be we can move it out too in future.

You're right, it's always had logic to do filtering based on presto_view flag, but I forgot that it also has special logic based on whether or not we're translating Hive views. In that case, maybe it is required to add more logic to ThriftHiveMetastore. Though FileHiveMetastore doesn't have any today, so I'd need to be more convinced for that one.

electrum · 2020-10-22T22:23:53Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

@@ -343,6 +343,14 @@ public ThriftMetastoreStats getStats()
                    .stopOnIllegalExceptions()
                    .run("getTable", stats.getGetTable().wrap(() -> {
                        Table table = getTableFromMetastore(identity, databaseName, tableName);
+
+                        // Check if hive view is supported
+                        if (!translateHiveViews


Similar to the other comment, this check should be done in HiveMetadata where the translation occurs.

electrum · 2020-10-22T22:30:08Z

presto-hive/src/test/java/io/prestosql/plugin/hive/AbstractTestHive.java

+    {
+        String tableName = "test_hive_view";
+        ConnectorSession session = newSession();
+        Table table = Table.builder()


We shouldn't need to create a new Hive view here, as these tests already create presto_test_view (referenced by the view field in this class). It should be enough to call

metadata.getView(session, view);

electrum · 2020-10-22T22:35:47Z

The view translation logic should be independent of the metastore implementation (which is a "dumb" store in terms of view data) and thus so should the tests. The AbstractTestHive tests are ancient and it would be nice to deprecate them at some point. Doing this end-to-end with a product test is cleaner.

If it would make it easier, we could add a session property for this, then we could easily change it in the product test.

presto-docs/src/main/sphinx/connector/hive.rst

phd3 · 2020-10-23T04:32:45Z

@electrum can you please take a look again? I've simplified the testing based on slack discussion, and just testing with TestHive for now.

Test failures were unrelated, retriggered to get a clean build.

findepi · 2020-10-23T09:08:56Z

The view translation logic should be independent of the metastore implementation (which is a "dumb" store in terms of view data) and thus so should the tests. The AbstractTestHive tests are ancient and it would be nice to deprecate them at some point. Doing this end-to-end with a product test is cleaner.

I agree, but note that product tests do not involve Glue today (#5426).
The Hive view support translation layer is independent from metastore, but listing of tables and views is.
https://github.com/prestosql/presto/blob/58ee2329fe769042b72974e5b683332e09fc973e/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java#L933-L942

I believe we do need both: product tests for "nice tests" and AbstractTestHive to cover Glue too until we do #5426.

since this is not a session property, I think it's currently hard to test in the product tests without adding a separate config file. But I'm not sure a different configuration is worth the overhead in product test framework.

If it would make it easier, we could add a session property for this, then we could easily change it in the product test.

@phd3 @electrum
adding a new catalog in product tests is really cheap, perhaps just one new file, the catalog configuration.
We should add a session property if we want this to be configurable on per-query basis.
I think this is a good idea, but I really want this not to be a result of our testing (in)abilities.

presto-docs/src/main/sphinx/connector/hive.rst

presto-hive-hadoop2/src/test/java/io/prestosql/plugin/hive/TestHive.java

phd3 · 2020-10-23T14:17:12Z

Two unrelated failures: #4173 and the one fixed in #5671. Rebasing and rerunning the jobs.

cla-bot bot added the cla-signed label Oct 21, 2020

phd3 added the WIP label Oct 21, 2020

phd3 force-pushed the hive-view-config-enforce branch from b290bb2 to c151153 Compare October 21, 2020 17:54

kokosing reviewed Oct 21, 2020

View reviewed changes

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java Show resolved Hide resolved

findepi mentioned this pull request Oct 21, 2020

Remove unused HiveViewNotSupportedException class #5599

Closed

phd3 force-pushed the hive-view-config-enforce branch 2 times, most recently from 359532b to ed776a0 Compare October 22, 2020 03:21

kokosing reviewed Oct 22, 2020

View reviewed changes

phd3 force-pushed the hive-view-config-enforce branch from ed776a0 to 3fc2b0e Compare October 22, 2020 17:25

phd3 removed the WIP label Oct 22, 2020

phd3 requested a review from kokosing October 22, 2020 21:29

martint requested a review from electrum October 22, 2020 21:45

electrum requested changes Oct 22, 2020

View reviewed changes

phd3 force-pushed the hive-view-config-enforce branch from 3fc2b0e to 7460b57 Compare October 23, 2020 00:40

phd3 commented Oct 23, 2020

View reviewed changes

presto-docs/src/main/sphinx/connector/hive.rst Show resolved Hide resolved

phd3 requested a review from electrum October 23, 2020 04:29

electrum reviewed Oct 23, 2020

View reviewed changes

presto-docs/src/main/sphinx/connector/hive.rst Outdated Show resolved Hide resolved

electrum approved these changes Oct 23, 2020

View reviewed changes

findepi reviewed Oct 23, 2020

View reviewed changes

presto-hive-hadoop2/src/test/java/io/prestosql/plugin/hive/TestHive.java Outdated Show resolved Hide resolved

phd3 force-pushed the hive-view-config-enforce branch from 7460b57 to 97c0a12 Compare October 23, 2020 14:17

phd3 added 2 commits October 23, 2020 09:21

Enforce hive.translate-hive-views propertry

7be6a0f

Add documentation for hive.translate-hive-views

20961dc

phd3 force-pushed the hive-view-config-enforce branch from 97c0a12 to 20961dc Compare October 23, 2020 14:24

martint added this to the 345 milestone Oct 23, 2020

martint merged commit f4eb7f0 into trinodb:master Oct 23, 2020

phd3 deleted the hive-view-config-enforce branch November 1, 2020 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce hive.translate-hive-views propertry #5636

Enforce hive.translate-hive-views propertry #5636

phd3 commented Oct 21, 2020

laurachenyu commented Oct 21, 2020

phd3 commented Oct 22, 2020

kokosing left a comment •

edited

Loading

kokosing Oct 22, 2020

phd3 commented Oct 22, 2020 •

edited

Loading

electrum Oct 22, 2020

phd3 Oct 22, 2020

electrum Oct 23, 2020

electrum Oct 22, 2020

electrum Oct 22, 2020

electrum commented Oct 22, 2020

phd3 commented Oct 23, 2020

findepi commented Oct 23, 2020

phd3 commented Oct 23, 2020

Enforce hive.translate-hive-views propertry #5636

Enforce hive.translate-hive-views propertry #5636

Conversation

phd3 commented Oct 21, 2020

laurachenyu commented Oct 21, 2020

phd3 commented Oct 22, 2020

kokosing left a comment • edited Loading

Choose a reason for hiding this comment

kokosing Oct 22, 2020

Choose a reason for hiding this comment

phd3 commented Oct 22, 2020 • edited Loading

electrum Oct 22, 2020

Choose a reason for hiding this comment

phd3 Oct 22, 2020

Choose a reason for hiding this comment

electrum Oct 23, 2020

Choose a reason for hiding this comment

electrum Oct 22, 2020

Choose a reason for hiding this comment

electrum Oct 22, 2020

Choose a reason for hiding this comment

electrum commented Oct 22, 2020

phd3 commented Oct 23, 2020

findepi commented Oct 23, 2020

phd3 commented Oct 23, 2020

kokosing left a comment •

edited

Loading

phd3 commented Oct 22, 2020 •

edited

Loading