-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Disallow querying iceberg tables in hive #10441
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
package io.trino.plugin.hive; | ||
|
||
import io.trino.plugin.hive.authentication.HiveIdentity; | ||
import io.trino.plugin.hive.metastore.Table; | ||
import io.trino.spi.connector.ColumnMetadata; | ||
import io.trino.spi.connector.ConnectorSession; | ||
import io.trino.spi.connector.ConnectorTableMetadata; | ||
|
@@ -22,6 +23,7 @@ | |
import io.trino.spi.connector.SchemaTableName; | ||
import io.trino.spi.connector.SystemTable; | ||
import io.trino.spi.type.Type; | ||
import io.trino.spi.type.TypeManager; | ||
|
||
import javax.inject.Inject; | ||
|
||
|
@@ -33,7 +35,15 @@ | |
import static com.google.common.collect.ImmutableList.toImmutableList; | ||
import static com.google.common.collect.ImmutableMap.toImmutableMap; | ||
import static com.google.common.collect.Streams.stream; | ||
import static io.trino.plugin.hive.HiveSessionProperties.getTimestampPrecision; | ||
import static io.trino.plugin.hive.SystemTableHandler.PARTITIONS; | ||
import static io.trino.plugin.hive.metastore.MetastoreUtil.getProtectMode; | ||
import static io.trino.plugin.hive.metastore.MetastoreUtil.verifyOnline; | ||
import static io.trino.plugin.hive.util.HiveBucketing.getHiveBucketHandle; | ||
import static io.trino.plugin.hive.util.HiveUtil.getPartitionKeyColumnHandles; | ||
import static io.trino.plugin.hive.util.HiveUtil.getRegularColumnHandles; | ||
import static io.trino.plugin.hive.util.HiveUtil.isDeltaLakeTable; | ||
import static io.trino.plugin.hive.util.HiveUtil.isIcebergTable; | ||
import static io.trino.plugin.hive.util.SystemTables.createSystemTable; | ||
import static java.util.Objects.requireNonNull; | ||
import static java.util.function.Function.identity; | ||
|
@@ -43,11 +53,13 @@ public class PartitionsSystemTableProvider | |
implements SystemTableProvider | ||
{ | ||
private final HivePartitionManager partitionManager; | ||
private final TypeManager typeManager; | ||
|
||
@Inject | ||
public PartitionsSystemTableProvider(HivePartitionManager partitionManager) | ||
public PartitionsSystemTableProvider(HivePartitionManager partitionManager, TypeManager typeManager) | ||
{ | ||
this.partitionManager = requireNonNull(partitionManager, "partitionManager is null"); | ||
this.typeManager = requireNonNull(typeManager, "typeManager is null"); | ||
} | ||
|
||
@Override | ||
|
@@ -68,11 +80,20 @@ public Optional<SystemTable> getSystemTable(HiveMetadata metadata, ConnectorSess | |
} | ||
|
||
SchemaTableName sourceTableName = PARTITIONS.getSourceTableName(tableName); | ||
HiveTableHandle sourceTableHandle = metadata.getTableHandle(session, sourceTableName); | ||
|
||
if (sourceTableHandle == null) { | ||
Table sourceTable = metadata.getMetastore() | ||
.getTable(new HiveIdentity(session), sourceTableName.getSchemaName(), sourceTableName.getTableName()) | ||
.orElse(null); | ||
if (sourceTable == null || isDeltaLakeTable(sourceTable) || isIcebergTable(sourceTable)) { | ||
return Optional.empty(); | ||
} | ||
verifyOnline(sourceTableName, Optional.empty(), getProtectMode(sourceTable), sourceTable.getParameters()); | ||
HiveTableHandle sourceTableHandle = new HiveTableHandle( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NOTE that compared to
|
||
sourceTableName.getSchemaName(), | ||
sourceTableName.getTableName(), | ||
sourceTable.getParameters(), | ||
getPartitionKeyColumnHandles(sourceTable, typeManager), | ||
getRegularColumnHandles(sourceTable, typeManager, getTimestampPrecision(session)), | ||
getHiveBucketHandle(session, sourceTable, typeManager)); | ||
|
||
List<HiveColumnHandle> partitionColumns = sourceTableHandle.getPartitionColumns(); | ||
if (partitionColumns.isEmpty()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,4 +61,10 @@ public void testHideDeltaLakeTables() | |
{ | ||
throw new SkipException("not supported"); | ||
} | ||
|
||
@Override | ||
public void testDisallowQueryingOfIcebergTables() | ||
{ | ||
throw new SkipException("not supported"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why disabled? add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While trying to run the test I receive
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which directory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the comment to contain the missing directory path. Note that the similar test |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -230,9 +230,9 @@ public void testDropTable() | |
String icebergTableName = "iceberg.default." + tableName; | ||
|
||
createIcebergTable(icebergTableName, false); | ||
onTrino().executeQuery("DROP TABLE " + hiveTableName); | ||
assertQueryFailure(() -> onTrino().executeQuery("TABLE " + icebergTableName)) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): line 1:1: Table '" + icebergTableName + "' does not exist"); | ||
//TODO restore test assertions after adding redirection awareness to the DropTableTask | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we have an issue to link to? (nit: add space after |
||
assertQueryFailure(() -> onTrino().executeQuery("DROP TABLE " + hiveTableName)) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); | ||
} | ||
|
||
@Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS}) | ||
|
@@ -305,18 +305,11 @@ public void testAlterTableRename() | |
|
||
createIcebergTable(icebergTableName, false); | ||
|
||
onTrino().executeQuery("ALTER TABLE " + hiveTableName + " RENAME TO " + tableName + "_new"); | ||
//TODO restore test assertions after adding redirection awareness to the RenameTableTask | ||
assertQueryFailure(() -> onTrino().executeQuery("ALTER TABLE " + hiveTableName + " RENAME TO " + tableName + "_new")) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); | ||
|
||
assertQueryFailure(() -> onTrino().executeQuery("TABLE " + hiveTableName)) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): line 1:1: Table '" + hiveTableName + "' does not exist"); | ||
assertQueryFailure(() -> onTrino().executeQuery("TABLE " + icebergTableName)) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): line 1:1: Table '" + icebergTableName + "' does not exist"); | ||
|
||
assertResultsEqual( | ||
onTrino().executeQuery("TABLE " + icebergTableName + "_new"), | ||
onTrino().executeQuery("TABLE " + hiveTableName + "_new")); | ||
|
||
onTrino().executeQuery("DROP TABLE " + icebergTableName + "_new"); | ||
onTrino().executeQuery("DROP TABLE " + icebergTableName); | ||
} | ||
|
||
@Test(groups = {HIVE_ICEBERG_REDIRECTIONS, PROFILE_SPECIFIC_TESTS}) | ||
|
@@ -328,15 +321,9 @@ public void testAlterTableAddColumn() | |
|
||
createIcebergTable(icebergTableName, false); | ||
|
||
onTrino().executeQuery("ALTER TABLE " + hiveTableName + " ADD COLUMN some_new_column double"); | ||
|
||
// TODO: ALTER TABLE succeeded, but new column was not added | ||
Assertions.assertThat(onTrino().executeQuery("DESCRIBE " + icebergTableName).column(1)) | ||
.containsOnly("nationkey", "name", "regionkey", "comment"); | ||
|
||
assertResultsEqual( | ||
onTrino().executeQuery("TABLE " + icebergTableName), | ||
onTrino().executeQuery("SELECT * /*, NULL*/ FROM tpch.tiny.nation")); | ||
//TODO restore test assertions after adding redirection awareness to the AddColumnTask | ||
assertQueryFailure(() -> onTrino().executeQuery("ALTER TABLE " + hiveTableName + " ADD COLUMN some_new_column double")) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); | ||
|
||
onTrino().executeQuery("DROP TABLE " + icebergTableName); | ||
} | ||
|
@@ -353,11 +340,9 @@ public void testCommentTable() | |
assertTableComment("hive", "default", tableName).isNull(); | ||
assertTableComment("iceberg", "default", tableName).isNull(); | ||
|
||
onTrino().executeQuery("COMMENT ON TABLE " + hiveTableName + " IS 'This is my table, there are many like it but this one is mine'"); | ||
|
||
// TODO: COMMENT ON TABLE succeeded, but comment was not preserved | ||
assertTableComment("hive", "default", tableName).isNull(); | ||
assertTableComment("iceberg", "default", tableName).isNull(); | ||
//TODO restore test assertions after adding redirection awareness to the CommentTask | ||
assertQueryFailure(() -> onTrino().executeQuery("COMMENT ON TABLE " + hiveTableName + " IS 'This is my table, there are many like it but this one is mine'")) | ||
.hasMessageMatching("\\QQuery failed (#\\E\\S+\\Q): Cannot query Iceberg table 'default." + tableName + "'"); | ||
|
||
onTrino().executeQuery("DROP TABLE " + icebergTableName); | ||
} | ||
|
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.
for follow-up, this returns empty when table not found, while PropertiesSystemTableProvider throws in such case
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.
Actually
PropertiesSystemTableProvider
returnsOptional.empty()
as well.This is covered by the previous test 3fb545e#diff-92c37834248d69878a49876bfa440c9f6afbee04b1363d402a04cf7fddc7bb62R2905-R2913