Disallow querying iceberg tables in hive

[findinpath]

Hive connector cannot read from Iceberg tables reason
why querying such tables shouldn't be permitted within
the hive connector.
In case of trying to query an Iceberg table from the
hive connector provide a meaningful message to the user
about not supporting such an operation.

In case that the hive users don't want to see at all
the Iceberg tables, the property hive.hide-iceberg-tables
can be set for the hive connector to true.

Fixes #8693

[findinpath]

I found the same model also in the testHideDeltaLakeTables() and I am wondering whether it makes sense to do the skipping at the end of the method.
Can anyone explain the reasoning behind this ?

[losipiuk]

the assertion is so we do not forget to "uncomment" the test when we fix the tested code.
But actually why is this test supposed to fail?

[findepi]

seems testDisallowQueryingOfIcebergTables is expected to pass, so why the override?

[findinpath]

I'm not sure whether the strategy of using hive.hide-xxx-tables properties is that good.
Would it make sense to rather offer a generic property to hide any non-hive tables?

[losipiuk]

nit: propertiesTableName. Or jus tinline.

[losipiuk]

First and last commit LGTM (can you extract separate PR with those?)

[losipiuk]

First and last commit LGTM (can you extract separate PR with those?)

Or two separate PRs?

[sajjoseph]

While I understand the need for this change, shouldn't this PR - #10173 - be a better solution for production use?

[findinpath]

While I understand the need for this change, shouldn't this PR - #10173 - be a better solution for production use?

@sajjoseph I'd say that the redirect usage is a different use case than what this PR is trying to solve.

[findinpath]

First and last commit LGTM (can you extract separate PR with those?)

Or two separate PRs?

I've created a separate PR for disallowing the query of $properties delta lake table in hive here:

#10447

[findinpath]

As @sajjoseph (#10441 (comment)) and @findepi (#10441 (comment)) pointed out, the iceberg redirects are a better option to deal with iceberg tables from shared hive metastores.

[findepi]

@findinpath i think there is a misunderstanding about my comment #10441 (comment) .
I asked you to drop the hive.hide-iceberg-tables property, not to drop the PR.

We still want to properly reject Iceberg tables when redirects are not enabled, #8693
Same as we do for Delta tables (to be improved in #10447).

[findepi]

Throw "Cannot query Iceberg table" instead of making the system throw "table not found".

[findinpath]

Unfortunately this doesn't work well with the concept of redirection recently implemented in hive.
If we throw an exception while trying to get the table handle in Hive, the workflow used for in the analyzer for getting the table handle will stop abruptly before checking for redirections.

See https://github.com/trinodb/trino/blob/master/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java#L1517 for reference

The current logic first checks whether we're dealing with a materialized view, then with a view and only at the end with a table (that may contain redirections).

[findepi]

If we throw an exception while trying to get the table handle in Hive, the workflow used for in the analyzer for getting the table handle will stop abruptly before checking for redirections.

the redirections should be consulted first.

[findepi]

The current logic first checks whether we're dealing with a materialized view, then with a view and only at the end with a table (that may contain redirections).

An iceberg table is not a materialized view, nor a view, so get[Materialized]View should return Optional.empty for these calls.

[findinpath]

At the moment, when checking whether we're dealing with a materialized view or view there is subsequently made a call towards HiveMetadata#getTableHandle which would end up in throwing an exception instead of returning Optional.empty in case that an exception would be thrown (in a similar fashion as for delta lake tables).

[findinpath]

Changing the logic of the StatementAnalyzer is something I'd avoid to do in this case for retrieving first the table redirections.

For this reason, I have opted to partly replicate the logic of HiveMetadata#getTableHandle method in the PartitionsSystemTableProvider which offers me the possibility to do a fine grained handling in case of dealing with an iceberg/delta lake table.

One inconvenient of this approach is that we have duplicated logic in HiveMetadata and in PartitionsSystemTableProvider.

[findepi]

Changing the logic of the StatementAnalyzer is something I'd avoid to do in this case for retrieving first the table redirections.

Agreed, this is out of question.

[findepi]

why disabled?

add assertThatThrownBy(super:: ...

[findinpath]

While trying to run the test I receive

2022-01-11T03:30:02.708-0600 INFO Copying resource dir 'spark_bucketed_nation' to /var/folders/1q/y42hmc4s3yl38kp0t0q142_c0000gn/T/TestHiveInMemoryMetastore7495715074983273908


java.lang.IllegalArgumentException: Table directory does not exist

	at com.google.common.base.Preconditions.checkArgument(Preconditions.java:145)
	at io.trino.plugin.hive.metastore.thrift.InMemoryThriftMetastore.createTable(InMemoryThriftMetastore.java:190)
	at io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore.createTable(BridgingHiveMetastore.java:205)
	at io.trino.plugin.hive.AbstractTestHive.testDisallowQueryingOfIcebergTables(AbstractTestHive.java:2978)
	at io.trino.plugin.hive.TestHiveInMemoryMetastore.testDisallowQueryingOfIcebergTables(TestHiveInMemoryMetastore.java:68)

[findepi]

Which directory?
This sounds like a test setup problem. Does it mean TestHiveInMemoryMetastore cannot run a test that uses createTable?

[findinpath]

I updated the comment to contain the missing directory path.

Note that the similar test testHideDeltaLakeTables is also containing a not supported skip exception.

https://github.com/trinodb/trino/blob/master/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveInMemoryMetastore.java#L59-L63

[findinpath]

@losipiuk

.add(row("iceberg_table")) // TODO: should this be filtered out?

filtering out the entry from show tables would involve querying the properties metadata of each table.

Can I remove this TODO ?

[findinpath]

I've added a separate PR #10577 for extending the redirection awareness on the table tasks because the TestHiveRedirectionToIceberg tests were failing with the modification of HiveMetadata#getTableHandle() method when a Iceberg table is being queried.

The current PR has the tests from TestHiveRedirectionToIceberg currently failing. Please advise what would be the sane way to go further with this PR.

[findepi]

Disallow querying Delta Lake system tables

This is not the right commit title, since it was disallowed before.

[findinpath]

I thought to name it Disallow instead of Deny.
I considered that before it was denied - by throwing the exception and now it is disallowed. Any wording suggestions in this case?

[findepi]

for follow-up, this returns empty when table not found, while PropertiesSystemTableProvider throws in such case

[findinpath]

Actually PropertiesSystemTableProvider returns Optional.empty() as well.
This is covered by the previous test 3fb545e#diff-92c37834248d69878a49876bfa440c9f6afbee04b1363d402a04cf7fddc7bb62R2905-R2913

[findepi]

do we have an issue to link to?

(nit: add space after //)

[findinpath]

NOTE that compared to HiveMetadata#getTableHandle I didn't include here anymore the check

        // we must not allow system tables due to how permissions are checked in SystemTableAwareAccessControl
        if (getSourceTableNameFromSystemTable(systemTableProviders, tableName).isPresent()) {
            throw new TrinoException(HIVE_INVALID_METADATA, "Unexpected table present in Hive metastore: " + tableName);
        }

[findepi]

The force push diff (https://github.com/trinodb/trino/compare/3fb545ed8d4c40042966ec147923987ff79208f3..22c899a87226b5636191acc3e25cfc00be641f1b) is large.
What were the actual changes here?

[findinpath]

@findepi I changed the assertions in the test class testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveRedirectionToIceberg.java so that the tests are green.