-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Support properties system table in Hive #268
Conversation
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConnectorFactory.java
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveConnectorFactory.java
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/security/SystemTableAwareAccessControl.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.
Please squash commits.
presto-hive/src/main/java/io/prestosql/plugin/hive/security/SystemTableAwareAccessControl.java
Show resolved
Hide resolved
aa8993f
to
92f5810
Compare
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.
A few minor nits, otherwise looks good. Per the class rename comment, please squash the commits.
Please ping me after you have addressed these so that we can get this merged.
presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveIntegrationSmokeTest.java
Show resolved
Hide resolved
"CREATE TABLE test_show_properties" + | ||
" WITH (" + | ||
"format = 'orc', " + | ||
"partitioned_by = ARRAY[ 'SHIP_PRIORITY', 'ORDER_STATUS' ]," + |
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.
Nit: don't put spaces around array brackets. Also, use lowercase for the column names.
partitioned_by = ARRAY['ship_priority', 'order_status'],
orc_bloom_filter_columns = ARRAY['ship_priority', 'order_status'],
"FROM tpch.tiny.orders"; | ||
|
||
assertUpdate(createTable, "SELECT count(*) FROM orders"); | ||
BasicQueryInfo createTableQuery = ((DistributedQueryRunner) getQueryRunner()).getCoordinator().getQueryManager().getQueries().stream().filter(basicQueryInfo -> basicQueryInfo.getQuery().equals(createTable)).findFirst().get(); |
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.
This is a clever way to get the query ID and node version! However, there is a simpler way:
String queryId = (String) computeScalar("SELECT query_id FROM system.runtime.queries WHERE query LIKE 'CREATE TABLE test_show_properties%'");
String nodeVersion = (String) computeScalar("SELECT node_version FROM system.runtime.nodes WHERE coordinator");
assertUpdate(createTable, "SELECT count(*) FROM orders"); | ||
BasicQueryInfo createTableQuery = ((DistributedQueryRunner) getQueryRunner()).getCoordinator().getQueryManager().getQueries().stream().filter(basicQueryInfo -> basicQueryInfo.getQuery().equals(createTable)).findFirst().get(); | ||
String nodeVersion = ((DistributedQueryRunner) getQueryRunner()).getCoordinator().refreshNodes().getActiveNodes().stream().map(Node::getVersion).findAny().get(); | ||
assertQuery("SELECT * FROM \"test_show_properties$properties\"", "SELECT '" + "ship_priority,order_status" + "','" + "0.5" + "','" + createTableQuery.getQueryId().getId() + "','" + nodeVersion + "'"); |
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.
This is hard to read. Please format like
assertQuery(
"SELECT * FROM \"test_show_properties$properties\"",
format("SELECT 'ship_priority,order_status', 0.5, '%s', '%s', queryId, nodeVersion));
92f5810
to
78765e7
Compare
78765e7
to
11d1f54
Compare
Merged, thanks! |
Support Query syntax
SELECT * FROM "table_name$properties"
to return properties defined in hive metastoreRef prestodb/presto#10793
Fixes prestodb/presto#10611