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

Avoid including hive views in iceberg SHOW TABLES #8153

Merged
merged 2 commits into from
Jun 3, 2021
Merged

Avoid including hive views in iceberg SHOW TABLES #8153

merged 2 commits into from
Jun 3, 2021

Conversation

raunaqmorarka
Copy link
Member

No description provided.

@cla-bot cla-bot bot added the cla-signed label May 31, 2021
@raunaqmorarka raunaqmorarka mentioned this pull request May 31, 2021
Merged
findepi
findepi reviewed Jun 1, 2021
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

LGTM

Please add a test exercising Hive and Iceberg using same backing metastore.
This should be a product test to get good coverage of metastore-dependent interactions like this. (For example -- can getTablesWithParameter be used with TABLE_COMMENT property?)

@raunaqmorarka
Copy link
Member Author

LGTM

Please add a test exercising Hive and Iceberg using same backing metastore.
This should be a product test to get good coverage of metastore-dependent interactions like this. (For example -- can getTablesWithParameter be used with TABLE_COMMENT property?)

Thanks for pointing out, turns out TABLE_PARAMETER_SAFE_VALUE_PATTERN blocks our parameter value, i'll need to tweak that a bit

@findepi
Copy link
Member

findepi commented Jun 1, 2021

Your value is not far from matching TABLE_PARAMETER_SAFE_VALUE_PATTERN

based on the code comment:

the HMS may want the parameter
* value to follow a Java regex pattern or a SQL pattern

seems like we could allow spaces in TABLE_PARAMETER_SAFE_VALUE_PATTERN.

@raunaqmorarka
Copy link
Member Author

Your value is not far from matching TABLE_PARAMETER_SAFE_VALUE_PATTERN

based on the code comment:

the HMS may want the parameter

  • value to follow a Java regex pattern or a SQL pattern

seems like we could allow spaces in TABLE_PARAMETER_SAFE_VALUE_PATTERN.

I got things working for iceberg by allowing spaces in TABLE_PARAMETER_SAFE_VALUE_PATTERN.
Also added a product test.
But I also see that in shared metastore setup, hive connector lists all the iceberg tables and MVs with same schema name because it calls getAllTables
In shared setup, are we mainly concerned about the iceberg side or the hive side listings also need to work correctly ?

anjalinorwood
anjalinorwood approved these changes Jun 2, 2021
View reviewed changes
Copy link
Member

@anjalinorwood anjalinorwood left a comment

Choose a reason for hiding this comment

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

LGTM!

sopel39
sopel39 reviewed Jun 2, 2021
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetadataListing.java
}

@Test
public void testTableColumnListing()
{
// Verify information_schema.columns does not include columns from non-Iceberg tables
assertQuery("SELECT table_name, column_name FROM iceberg.information_schema.columns WHERE table_schema = 'test_schema'",
"VALUES ('iceberg_table1', '_string'), ('iceberg_table1', '_integer'), ('iceberg_table2', '_double')");
// TODO this should include columns from the materialized view as well
Copy link
Member

Choose a reason for hiding this comment

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

please create an issue in Github and reference it here

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 should get solved by #8113

...oduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergHiveMetadataListing.java Outdated Show resolved Hide resolved
...oduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergHiveMetadataListing.java Outdated Show resolved Hide resolved
...oduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergHiveMetadataListing.java
onTrino().executeQuery("CREATE TABLE iceberg.default.iceberg_table1 (_string VARCHAR, _integer INTEGER)");
onTrino().executeQuery("CREATE MATERIALIZED VIEW iceberg.default.iceberg_materialized_view AS " +
"SELECT * FROM iceberg.default.iceberg_table1");
storageTable = getOnlyElement(onTrino().executeQuery("SHOW TABLES FROM iceberg.default")
Copy link
Member

Choose a reason for hiding this comment

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

is storage table created immediately?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, in iceberg CREATE MV we create the storage table even before storing the MV definition in HMS

@sopel39 sopel39 merged commit e326ab9 into trinodb:master Jun 3, 2021
@sopel39 sopel39 mentioned this pull request Jun 3, 2021
Closed
13 tasks
@raunaqmorarka raunaqmorarka deleted the ice-mv-list-fix branch June 24, 2021 07:03
@yingsu00 yingsu00 mentioned this pull request Aug 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anjalinorwood anjalinorwood anjalinorwood approved these changes

@electrum electrum Awaiting requested review from electrum

@findepi findepi Awaiting requested review from findepi

@sopel39 sopel39 Awaiting requested review from sopel39

Assignees
No one assigned
Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@raunaqmorarka @findepi @sopel39 @anjalinorwood