From 0654e2ffb4422ccce3b2e98189cf3d03a9e5d96f Mon Sep 17 00:00:00 2001 From: pratyakshsharma Date: Fri, 4 Oct 2024 16:45:05 +0530 Subject: [PATCH 1/3] Add ALTER TABLE SET PROPERTIES statement Cherry-pick of https://github.com/trinodb/trino/pull/9401/commits/72df2b94a15343498c29133dadec68e2881d258e Co-authored-by: ebyhr --- .../sql/analyzer/utils/StatementUtils.java | 2 + .../src/main/sphinx/sql/alter-table.rst | 5 + .../hive/security/LegacyAccessControl.java | 6 + .../security/SqlStandardAccessControl.java | 20 +++ .../SystemTableAwareAccessControl.java | 7 + .../presto/execution/SetPropertiesTask.java | 95 ++++++++++++++ .../metadata/AbstractPropertyManager.java | 36 +++++- .../metadata/DelegatingMetadataManager.java | 5 + .../facebook/presto/metadata/Metadata.java | 5 + .../presto/metadata/MetadataManager.java | 8 ++ .../presto/metadata/MetadataUtil.java | 7 + .../presto/security/AccessControlManager.java | 13 ++ .../security/AllowAllSystemAccessControl.java | 4 + .../FileBasedSystemAccessControl.java | 8 ++ .../presto/testing/LocalQueryRunner.java | 3 + .../testing/TestingAccessControlManager.java | 17 ++- .../util/PrestoDataDefBindingHelper.java | 3 + .../connector/MockConnectorFactory.java | 4 + .../presto/metadata/AbstractMockMetadata.java | 6 + .../TestFileBasedSystemAccessControl.java | 5 + .../com/facebook/presto/sql/parser/SqlBase.g4 | 2 + .../com/facebook/presto/sql/SqlFormatter.java | 22 ++++ .../presto/sql/parser/AstBuilder.java | 17 +++ .../facebook/presto/sql/tree/AstVisitor.java | 5 + .../presto/sql/tree/SetProperties.java | 120 ++++++++++++++++++ .../presto/sql/parser/TestSqlParser.java | 15 +++ .../sql/parser/TestStatementBuilder.java | 4 + .../base/security/AllowAllAccessControl.java | 6 + .../base/security/FileBasedAccessControl.java | 10 ++ .../ForwardingConnectorAccessControl.java | 7 + .../ForwardingSystemAccessControl.java | 6 + .../security/TestFileBasedAccessControl.java | 4 + .../src/test/resources/table.json | 6 + .../spi/connector/ConnectorAccessControl.java | 12 ++ .../spi/connector/ConnectorMetadata.java | 5 + .../ClassLoaderSafeConnectorMetadata.java | 7 + .../presto/spi/security/AccessControl.java | 8 ++ .../spi/security/AccessDeniedException.java | 10 ++ .../spi/security/AllowAllAccessControl.java | 6 + .../spi/security/DenyAllAccessControl.java | 8 ++ .../spi/security/SystemAccessControl.java | 11 ++ 41 files changed, 543 insertions(+), 7 deletions(-) create mode 100644 presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java create mode 100644 presto-parser/src/main/java/com/facebook/presto/sql/tree/SetProperties.java diff --git a/presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java b/presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java index 797ab5dcf614..0a2b34bebef1 100644 --- a/presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java +++ b/presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java @@ -55,6 +55,7 @@ import com.facebook.presto.sql.tree.Revoke; import com.facebook.presto.sql.tree.RevokeRoles; import com.facebook.presto.sql.tree.Rollback; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetRole; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.ShowCatalogs; @@ -142,6 +143,7 @@ private StatementUtils() {} builder.put(DropFunction.class, QueryType.CONTROL); builder.put(Use.class, QueryType.CONTROL); builder.put(SetSession.class, QueryType.CONTROL); + builder.put(SetProperties.class, QueryType.DATA_DEFINITION); builder.put(ResetSession.class, QueryType.CONTROL); builder.put(StartTransaction.class, QueryType.CONTROL); builder.put(Commit.class, QueryType.CONTROL); diff --git a/presto-docs/src/main/sphinx/sql/alter-table.rst b/presto-docs/src/main/sphinx/sql/alter-table.rst index abb3e94b2eeb..451b4dd2794b 100644 --- a/presto-docs/src/main/sphinx/sql/alter-table.rst +++ b/presto-docs/src/main/sphinx/sql/alter-table.rst @@ -14,6 +14,7 @@ Synopsis ALTER TABLE [ IF EXISTS ] name ADD [ CONSTRAINT constraint_name ] { PRIMARY KEY | UNIQUE } ( { column_name [, ...] } ) [ { ENABLED | DISABLED } ] [ [ NOT ] RELY ] [ [ NOT ] ENFORCED } ] ALTER TABLE [ IF EXISTS ] name DROP CONSTRAINT [ IF EXISTS ] constraint_name ALTER TABLE [ IF EXISTS ] name ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL + ALTER TABLE [ IF EXISTS ] name SET PROPERTIES (property_name=value, [, ...]) Description ----------- @@ -89,6 +90,10 @@ Drop not null constraint from column ``zip`` in the ``users`` table:: ALTER TABLE users ALTER COLUMN zip DROP NOT NULL; +Set table property (``x=y``) to table ``users``:: + + ALTER TABLE users SET PROPERTIES (x='y'); + See Also -------- diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/security/LegacyAccessControl.java b/presto-hive/src/main/java/com/facebook/presto/hive/security/LegacyAccessControl.java index a4a971ab2d69..2277d1dab0f5 100644 --- a/presto-hive/src/main/java/com/facebook/presto/hive/security/LegacyAccessControl.java +++ b/presto-hive/src/main/java/com/facebook/presto/hive/security/LegacyAccessControl.java @@ -29,6 +29,7 @@ import javax.inject.Inject; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -101,6 +102,11 @@ public void checkCanCreateTable(ConnectorTransactionHandle transaction, Connecto { } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanDropTable(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName) { diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java b/presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java index 44447422f154..6fd805e34c5e 100644 --- a/presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java +++ b/presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java @@ -33,6 +33,7 @@ import javax.inject.Inject; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -74,6 +75,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denySelectTable; import static com.facebook.presto.spi.security.AccessDeniedException.denySetCatalogSessionProperty; import static com.facebook.presto.spi.security.AccessDeniedException.denySetRole; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowRoles; import static com.facebook.presto.spi.security.AccessDeniedException.denyTruncateTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyUpdateTableColumns; @@ -186,6 +188,24 @@ public void checkCanCreateTable(ConnectorTransactionHandle transaction, Connecto } } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + MetastoreContext metastoreContext = new MetastoreContext(identity, + context.getQueryId().getId(), + context.getClientInfo(), + context.getClientTags(), + context.getSource(), + Optional.empty(), + false, + HiveColumnConverterProvider.DEFAULT_COLUMN_CONVERTER_PROVIDER, + context.getWarningCollector(), + context.getRuntimeStats()); + if (!isTableOwner(transactionHandle, identity, metastoreContext, tableName)) { + denySetTableProperties(tableName.toString()); + } + } + @Override public void checkCanDropTable(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName) { diff --git a/presto-hive/src/main/java/com/facebook/presto/hive/security/SystemTableAwareAccessControl.java b/presto-hive/src/main/java/com/facebook/presto/hive/security/SystemTableAwareAccessControl.java index b6271dec0a73..9f2b182d39d4 100644 --- a/presto-hive/src/main/java/com/facebook/presto/hive/security/SystemTableAwareAccessControl.java +++ b/presto-hive/src/main/java/com/facebook/presto/hive/security/SystemTableAwareAccessControl.java @@ -25,6 +25,7 @@ import com.facebook.presto.spi.security.PrestoPrincipal; import com.facebook.presto.spi.security.Privilege; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -83,6 +84,12 @@ public void checkCanCreateTable(ConnectorTransactionHandle transactionHandle, Co delegate.checkCanCreateTable(transactionHandle, identity, context, tableName); } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + delegate.checkCanSetTableProperties(transactionHandle, identity, context, tableName, properties); + } + @Override public void checkCanDropTable(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName) { diff --git a/presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java b/presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java new file mode 100644 index 000000000000..d9ccb217bfe5 --- /dev/null +++ b/presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java @@ -0,0 +1,95 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.execution; + +import com.facebook.presto.Session; +import com.facebook.presto.common.QualifiedObjectName; +import com.facebook.presto.metadata.Metadata; +import com.facebook.presto.metadata.MetadataUtil; +import com.facebook.presto.spi.PrestoException; +import com.facebook.presto.spi.TableHandle; +import com.facebook.presto.spi.WarningCollector; +import com.facebook.presto.spi.security.AccessControl; +import com.facebook.presto.sql.tree.Expression; +import com.facebook.presto.sql.tree.SetProperties; +import com.facebook.presto.transaction.TransactionManager; +import com.google.common.util.concurrent.ListenableFuture; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; +import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; +import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED; +import static com.facebook.presto.sql.NodeUtils.mapFromProperties; +import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor; +import static com.facebook.presto.sql.tree.SetProperties.Type.TABLE; +import static com.google.common.util.concurrent.Futures.immediateFuture; +import static java.lang.String.format; + +public class SetPropertiesTask + implements DDLDefinitionTask +{ + @Override + public String getName() + { + return "SET PROPERTIES"; + } + + @Override + public ListenableFuture execute(SetProperties statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List parameters, WarningCollector warningCollector) + { + QualifiedObjectName tableName = MetadataUtil.createQualifiedObjectName(session, statement, statement.getTableName()); + Map sqlProperties = mapFromProperties(statement.getProperties()); + + if (statement.getType() == TABLE) { + Map properties = metadata.getTablePropertyManager().getUserSpecifiedProperties( + getConnectorIdOrThrow(session, metadata, tableName.getCatalogName()), + tableName.getCatalogName(), + sqlProperties, + session, + metadata, + parameterExtractor(statement, parameters)).build(); + setTableProperties(statement, tableName, metadata, accessControl, session, properties); + } + else { + throw new PrestoException(NOT_SUPPORTED, format("Unsupported target type: %s", statement.getType())); + } + + return immediateFuture(null); + } + + private void setTableProperties(SetProperties statement, QualifiedObjectName tableName, Metadata metadata, AccessControl accessControl, Session session, Map properties) + { + if (metadata.getMetadataResolver(session).getMaterializedView(tableName).isPresent()) { + throw new PrestoException(NOT_SUPPORTED, "Cannot set table properties to a materialized view"); + } + + if (metadata.getMetadataResolver(session).getView(tableName).isPresent()) { + throw new PrestoException(NOT_SUPPORTED, "Cannot set table properties to a view"); + } + + Optional tableHandle = metadata.getMetadataResolver(session).getTableHandle(tableName); + if (!tableHandle.isPresent()) { + if (!statement.isTableExists()) { + throw new PrestoException(NOT_FOUND, format("Table does not exist: %s", tableName)); + } + return; + } + + accessControl.checkCanSetTableProperties(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), tableName, properties); + metadata.setTableProperties(session, tableHandle.get(), properties); + } +} diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/AbstractPropertyManager.java b/presto-main/src/main/java/com/facebook/presto/metadata/AbstractPropertyManager.java index 630713273423..11b4b805d6a5 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/AbstractPropertyManager.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/AbstractPropertyManager.java @@ -70,7 +70,7 @@ public final void removeProperties(ConnectorId connectorId) connectorProperties.remove(connectorId); } - public final Map getProperties( + public final ImmutableMap.Builder getUserSpecifiedProperties( ConnectorId connectorId, String catalog, // only use this for error messages Map sqlPropertyValues, @@ -78,11 +78,7 @@ public final Map getProperties( Metadata metadata, Map, Expression> parameters) { - Map> supportedProperties = connectorProperties.get(connectorId); - if (supportedProperties == null) { - throw new PrestoException(NOT_FOUND, "Catalog not found: " + catalog); - } - + Map> supportedProperties = getSupportedProperties(connectorId, catalog); ImmutableMap.Builder properties = ImmutableMap.builder(); // Fill in user-specified properties @@ -125,6 +121,25 @@ public final Map getProperties( properties.put(property.getName(), value); } + return properties; + } + + public final Map getProperties( + ConnectorId connectorId, + String catalog, // only use this for error messages + Map sqlPropertyValues, + Session session, + Metadata metadata, + Map, Expression> parameters) + { + Map> supportedProperties = getSupportedProperties(connectorId, catalog); + ImmutableMap.Builder properties = getUserSpecifiedProperties( + connectorId, + catalog, + sqlPropertyValues, + session, + metadata, + parameters); Map userSpecifiedProperties = properties.build(); // Fill in the remaining properties with non-null defaults @@ -144,6 +159,15 @@ public Map>> getAllProperties() return ImmutableMap.copyOf(connectorProperties); } + private Map> getSupportedProperties(ConnectorId connectorId, String catalog) + { + Map> supportedProperties = connectorProperties.get(connectorId); + if (supportedProperties == null) { + throw new PrestoException(NOT_FOUND, "Catalog not found: " + catalog); + } + return supportedProperties; + } + private Object evaluatePropertyValue(Expression expression, Type expectedType, Session session, Metadata metadata, Map, Expression> parameters) { Expression rewritten = ExpressionTreeRewriter.rewriteWith(new ParameterRewriter(parameters), expression); diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/DelegatingMetadataManager.java b/presto-main/src/main/java/com/facebook/presto/metadata/DelegatingMetadataManager.java index cfc65f6bd61f..528a48f579c0 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/DelegatingMetadataManager.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/DelegatingMetadataManager.java @@ -238,6 +238,11 @@ public void renameTable(Session session, TableHandle tableHandle, QualifiedObjec delegate.renameTable(session, tableHandle, newTableName); } + public void setTableProperties(Session session, TableHandle tableHandle, Map properties) + { + delegate.setTableProperties(session, tableHandle, properties); + } + @Override public void renameColumn(Session session, TableHandle tableHandle, ColumnHandle source, String target) { diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java b/presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java index 01679c190098..7c2653a99e0f 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/Metadata.java @@ -211,6 +211,11 @@ public interface Metadata */ void renameTable(Session session, TableHandle tableHandle, QualifiedObjectName newTableName); + /** + * Set properties to the specified table. + */ + void setTableProperties(Session session, TableHandle tableHandle, Map properties); + /** * Rename the specified column. */ diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java index c9b86244de8c..cecb85dc8f94 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataManager.java @@ -673,6 +673,14 @@ public void renameTable(Session session, TableHandle tableHandle, QualifiedObjec metadata.renameTable(session.toConnectorSession(connectorId), tableHandle.getConnectorHandle(), toSchemaTableName(newTableName)); } + @Override + public void setTableProperties(Session session, TableHandle tableHandle, Map properties) + { + ConnectorId connectorId = tableHandle.getConnectorId(); + ConnectorMetadata metadata = getMetadataForWrite(session, connectorId); + metadata.setTableProperties(session.toConnectorSession(connectorId), tableHandle.getConnectorHandle(), properties); + } + @Override public void renameColumn(Session session, TableHandle tableHandle, ColumnHandle source, String target) { diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java index 243eb92717a9..b00783903ad9 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java @@ -40,6 +40,7 @@ import java.util.List; import java.util.Optional; +import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; import static com.facebook.presto.spi.StandardErrorCode.SYNTAX_ERROR; import static com.facebook.presto.spi.security.PrincipalType.ROLE; import static com.facebook.presto.spi.security.PrincipalType.USER; @@ -84,6 +85,12 @@ public static SchemaTableName toSchemaTableName(QualifiedObjectName qualifiedObj return new SchemaTableName(qualifiedObjectName.getSchemaName(), qualifiedObjectName.getObjectName()); } + public static ConnectorId getConnectorIdOrThrow(Session session, Metadata metadata, String catalogName) + { + return metadata.getCatalogHandle(session, catalogName) + .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + catalogName)); + } + public static String checkLowerCase(String value, String name) { if (value == null) { diff --git a/presto-main/src/main/java/com/facebook/presto/security/AccessControlManager.java b/presto-main/src/main/java/com/facebook/presto/security/AccessControlManager.java index 011db98e315a..d4d0125959c5 100644 --- a/presto-main/src/main/java/com/facebook/presto/security/AccessControlManager.java +++ b/presto-main/src/main/java/com/facebook/presto/security/AccessControlManager.java @@ -337,6 +337,19 @@ public void checkCanRenameTable(TransactionId transactionId, Identity identity, } } + @Override + public void checkCanSetTableProperties(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, Map properties) + { + requireNonNull(identity, "identity is null"); + requireNonNull(tableName, "tableName is null"); + authenticationCheck(() -> checkCanAccessCatalog(identity, context, tableName.getCatalogName())); + authorizationCheck(() -> systemAccessControl.get().checkCanSetTableProperties(identity, context, toCatalogSchemaTableName(tableName))); + CatalogAccessControlEntry entry = getConnectorAccessControl(transactionId, tableName.getCatalogName()); + if (entry != null) { + authorizationCheck(() -> entry.getAccessControl().checkCanSetTableProperties(entry.getTransactionHandle(transactionId), identity.toConnectorIdentity(tableName.getCatalogName()), context, toSchemaTableName(tableName), properties)); + } + } + @Override public void checkCanShowTablesMetadata(TransactionId transactionId, Identity identity, AccessControlContext context, CatalogSchemaName schema) { diff --git a/presto-main/src/main/java/com/facebook/presto/security/AllowAllSystemAccessControl.java b/presto-main/src/main/java/com/facebook/presto/security/AllowAllSystemAccessControl.java index 01f260f64b6d..4197801047f7 100644 --- a/presto-main/src/main/java/com/facebook/presto/security/AllowAllSystemAccessControl.java +++ b/presto-main/src/main/java/com/facebook/presto/security/AllowAllSystemAccessControl.java @@ -122,6 +122,10 @@ public void checkCanCreateTable(Identity identity, AccessControlContext context, { } + public void checkCanSetTableProperties(Identity identity, AccessControlContext context, CatalogSchemaTableName table) + { + } + @Override public void checkCanDropTable(Identity identity, AccessControlContext context, CatalogSchemaTableName table) { diff --git a/presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java b/presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java index 8a4aab91a559..f9bf1d5b976d 100644 --- a/presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java +++ b/presto-main/src/main/java/com/facebook/presto/security/FileBasedSystemAccessControl.java @@ -66,6 +66,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denyRenameSchema; import static com.facebook.presto.spi.security.AccessDeniedException.denyRenameTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyRevokeTablePrivilege; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denySetUser; import static com.facebook.presto.spi.security.AccessDeniedException.denyTruncateTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyUpdateTableColumns; @@ -280,6 +281,13 @@ public void checkCanCreateTable(Identity identity, AccessControlContext context, } } + public void checkCanSetTableProperties(Identity identity, AccessControlContext context, CatalogSchemaTableName table) + { + if (!canAccessCatalog(identity, table.getCatalogName(), ALL)) { + denySetTableProperties(table.toString()); + } + } + @Override public void checkCanDropTable(Identity identity, AccessControlContext context, CatalogSchemaTableName table) { diff --git a/presto-main/src/main/java/com/facebook/presto/testing/LocalQueryRunner.java b/presto-main/src/main/java/com/facebook/presto/testing/LocalQueryRunner.java index d200afe8d916..3656b5d73493 100644 --- a/presto-main/src/main/java/com/facebook/presto/testing/LocalQueryRunner.java +++ b/presto-main/src/main/java/com/facebook/presto/testing/LocalQueryRunner.java @@ -73,6 +73,7 @@ import com.facebook.presto.execution.ResetSessionTask; import com.facebook.presto.execution.RollbackTask; import com.facebook.presto.execution.ScheduledSplit; +import com.facebook.presto.execution.SetPropertiesTask; import com.facebook.presto.execution.SetSessionTask; import com.facebook.presto.execution.StartTransactionTask; import com.facebook.presto.execution.TaskManagerConfig; @@ -211,6 +212,7 @@ import com.facebook.presto.sql.tree.RenameTable; import com.facebook.presto.sql.tree.ResetSession; import com.facebook.presto.sql.tree.Rollback; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.StartTransaction; import com.facebook.presto.sql.tree.Statement; @@ -583,6 +585,7 @@ private LocalQueryRunner(Session defaultSession, FeaturesConfig featuresConfig, .put(StartTransaction.class, new StartTransactionTask()) .put(Commit.class, new CommitTask()) .put(Rollback.class, new RollbackTask()) + .put(SetProperties.class, new SetPropertiesTask()) .build(); SpillerStats spillerStats = new SpillerStats(); diff --git a/presto-main/src/main/java/com/facebook/presto/testing/TestingAccessControlManager.java b/presto-main/src/main/java/com/facebook/presto/testing/TestingAccessControlManager.java index 86658c0d2992..1db74c076d0b 100644 --- a/presto-main/src/main/java/com/facebook/presto/testing/TestingAccessControlManager.java +++ b/presto-main/src/main/java/com/facebook/presto/testing/TestingAccessControlManager.java @@ -29,6 +29,7 @@ import java.security.Principal; import java.util.Collections; import java.util.HashSet; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -52,6 +53,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denySelectColumns; import static com.facebook.presto.spi.security.AccessDeniedException.denySetCatalogSessionProperty; import static com.facebook.presto.spi.security.AccessDeniedException.denySetSystemSessionProperty; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denySetUser; import static com.facebook.presto.spi.security.AccessDeniedException.denyTruncateTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyUpdateTableColumns; @@ -73,6 +75,7 @@ import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.RENAME_TABLE; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.SELECT_COLUMN; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.SET_SESSION; +import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.SET_TABLE_PROPERTIES; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.SET_USER; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.TRUNCATE_TABLE; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.UPDATE_TABLE; @@ -189,6 +192,18 @@ public void checkCanRenameTable(TransactionId transactionId, Identity identity, } } + @Override + public void checkCanSetTableProperties(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, Map properties) + { + if (shouldDenyPrivilege(identity.getUser(), tableName.getObjectName(), SET_TABLE_PROPERTIES)) { + denySetTableProperties(tableName.toString()); + } + + if (denyPrivileges.isEmpty()) { + super.checkCanSetTableProperties(transactionId, identity, context, tableName, properties); + } + } + @Override public void checkCanAddColumns(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName) { @@ -368,7 +383,7 @@ public enum TestingPrivilegeType CREATE_TABLE, DROP_TABLE, RENAME_TABLE, INSERT_TABLE, DELETE_TABLE, TRUNCATE_TABLE, UPDATE_TABLE, ADD_COLUMN, DROP_COLUMN, RENAME_COLUMN, SELECT_COLUMN, ADD_CONSTRAINT, DROP_CONSTRAINT, - CREATE_VIEW, DROP_VIEW, CREATE_VIEW_WITH_SELECT_COLUMNS, + CREATE_VIEW, DROP_VIEW, CREATE_VIEW_WITH_SELECT_COLUMNS, SET_TABLE_PROPERTIES, SET_SESSION } diff --git a/presto-main/src/main/java/com/facebook/presto/util/PrestoDataDefBindingHelper.java b/presto-main/src/main/java/com/facebook/presto/util/PrestoDataDefBindingHelper.java index ce4a41e5f091..f83e282373e0 100644 --- a/presto-main/src/main/java/com/facebook/presto/util/PrestoDataDefBindingHelper.java +++ b/presto-main/src/main/java/com/facebook/presto/util/PrestoDataDefBindingHelper.java @@ -46,6 +46,7 @@ import com.facebook.presto.execution.RevokeRolesTask; import com.facebook.presto.execution.RevokeTask; import com.facebook.presto.execution.RollbackTask; +import com.facebook.presto.execution.SetPropertiesTask; import com.facebook.presto.execution.SetRoleTask; import com.facebook.presto.execution.SetSessionTask; import com.facebook.presto.execution.StartTransactionTask; @@ -83,6 +84,7 @@ import com.facebook.presto.sql.tree.Revoke; import com.facebook.presto.sql.tree.RevokeRoles; import com.facebook.presto.sql.tree.Rollback; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetRole; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.StartTransaction; @@ -146,6 +148,7 @@ private PrestoDataDefBindingHelper() {} transactionDefBuilder.put(Commit.class, CommitTask.class); transactionDefBuilder.put(Rollback.class, RollbackTask.class); transactionDefBuilder.put(SetRole.class, SetRoleTask.class); + transactionDefBuilder.put(SetProperties.class, SetPropertiesTask.class); transactionDefBuilder.put(Prepare.class, PrepareTask.class); transactionDefBuilder.put(Deallocate.class, DeallocateTask.class); transactionDefBuilder.put(CreateFunction.class, CreateFunctionTask.class); diff --git a/presto-main/src/test/java/com/facebook/presto/connector/MockConnectorFactory.java b/presto-main/src/test/java/com/facebook/presto/connector/MockConnectorFactory.java index 58a1e343398f..6b12c9aac74f 100644 --- a/presto-main/src/test/java/com/facebook/presto/connector/MockConnectorFactory.java +++ b/presto-main/src/test/java/com/facebook/presto/connector/MockConnectorFactory.java @@ -179,6 +179,10 @@ public List listTables(ConnectorSession session, String schemaN return listTables.apply(session, schemaNameOrNull); } + public void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + } + @Override public Map getColumnHandles(ConnectorSession session, ConnectorTableHandle tableHandle) { diff --git a/presto-main/src/test/java/com/facebook/presto/metadata/AbstractMockMetadata.java b/presto-main/src/test/java/com/facebook/presto/metadata/AbstractMockMetadata.java index 5ba7b34b2d77..d1b296087357 100644 --- a/presto-main/src/test/java/com/facebook/presto/metadata/AbstractMockMetadata.java +++ b/presto-main/src/test/java/com/facebook/presto/metadata/AbstractMockMetadata.java @@ -283,6 +283,12 @@ public void renameTable(Session session, TableHandle tableHandle, QualifiedObjec throw new UnsupportedOperationException(); } + @Override + public void setTableProperties(Session session, TableHandle tableHandle, Map properties) + { + throw new UnsupportedOperationException(); + } + @Override public void renameColumn(Session session, TableHandle tableHandle, ColumnHandle source, String target) { diff --git a/presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java b/presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java index 9d0d574e7805..f9f661381e65 100644 --- a/presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java +++ b/presto-main/src/test/java/com/facebook/presto/security/TestFileBasedSystemAccessControl.java @@ -342,6 +342,7 @@ public void testTableOperations() throws IOException accessControlManager.checkCanSelectFromColumns(transactionId, alice, context, aliceTable, ImmutableSet.of()); accessControlManager.checkCanInsertIntoTable(transactionId, alice, context, aliceTable); accessControlManager.checkCanDeleteFromTable(transactionId, alice, context, aliceTable); + accessControlManager.checkCanSetTableProperties(transactionId, alice, context, aliceTable, ImmutableMap.of()); accessControlManager.checkCanAddColumns(transactionId, alice, context, aliceTable); accessControlManager.checkCanRenameColumn(transactionId, alice, context, aliceTable); }); @@ -385,6 +386,10 @@ public void testTableOperationsReadOnly() throws IOException accessControlManager.checkCanDeleteFromTable(transactionId, alice, context, aliceTable); })); + assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { + accessControlManager.checkCanSetTableProperties(transactionId, alice, context, aliceTable, ImmutableMap.of()); + })); + assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { accessControlManager.checkCanAddColumns(transactionId, alice, context, aliceTable); })); diff --git a/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 b/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 index f495ed0aab47..c5b80d73eb2b 100644 --- a/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 +++ b/presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 @@ -66,6 +66,8 @@ statement ALTER (COLUMN)? column=identifier SET NOT NULL #alterColumnSetNotNull | ALTER TABLE (IF EXISTS)? tableName=qualifiedName ALTER (COLUMN)? column=identifier DROP NOT NULL #alterColumnDropNotNull + | ALTER TABLE (IF EXISTS)? tableName=qualifiedName + SET PROPERTIES properties #setTableProperties | ANALYZE qualifiedName (WITH properties)? #analyze | CREATE TYPE qualifiedName AS ( '(' sqlParameterDeclaration (',' sqlParameterDeclaration)* ')' diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java b/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java index 4d73769e85ed..ea7c192cc859 100644 --- a/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java +++ b/presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java @@ -92,6 +92,7 @@ import com.facebook.presto.sql.tree.SampledRelation; import com.facebook.presto.sql.tree.Select; import com.facebook.presto.sql.tree.SelectItem; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetRole; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.ShowCatalogs; @@ -815,6 +816,27 @@ protected Void visitShowTables(ShowTables node, Integer context) return null; } + protected Void visitSetProperties(SetProperties node, Integer context) + { + builder.append("ALTER TABLE "); + if (node.isTableExists()) { + builder.append("IF EXISTS "); + } + builder.append(formatName(node.getTableName())); + builder.append(" SET PROPERTIES ( "); + builder.append(joinProperties(node.getProperties())); + builder.append(" )"); + return null; + } + + private String joinProperties(List properties) + { + return properties.stream() + .map(element -> formatExpression(element.getName(), Optional.empty()) + " = " + + formatExpression(element.getValue(), Optional.empty())) + .collect(joining(", ")); + } + @Override protected Void visitShowCreate(ShowCreate node, Integer context) { diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java b/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java index a004fffad308..f916feed3ad7 100644 --- a/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java +++ b/presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java @@ -141,6 +141,7 @@ import com.facebook.presto.sql.tree.SearchedCaseExpression; import com.facebook.presto.sql.tree.Select; import com.facebook.presto.sql.tree.SelectItem; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetRole; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.ShowCatalogs; @@ -209,6 +210,7 @@ import static com.facebook.presto.sql.tree.RoutineCharacteristics.NullCallClause; import static com.facebook.presto.sql.tree.RoutineCharacteristics.NullCallClause.CALLED_ON_NULL_INPUT; import static com.facebook.presto.sql.tree.RoutineCharacteristics.NullCallClause.RETURNS_NULL_ON_NULL_INPUT; +import static com.facebook.presto.sql.tree.SetProperties.Type.TABLE; import static com.facebook.presto.sql.tree.TableVersionExpression.TableVersionOperator; import static com.facebook.presto.sql.tree.TableVersionExpression.TableVersionOperator.EQUAL; import static com.facebook.presto.sql.tree.TableVersionExpression.TableVersionOperator.LESS_THAN; @@ -457,6 +459,21 @@ public Node visitRenameTable(SqlBaseParser.RenameTableContext context) return new RenameTable(getLocation(context), getQualifiedName(context.from), getQualifiedName(context.to), context.EXISTS() != null); } + @Override + public Node visitSetTableProperties(SqlBaseParser.SetTablePropertiesContext context) + { + List properties = ImmutableList.of(); + if (context.properties() != null) { + properties = visit(context.properties().property(), Property.class); + } + + return new SetProperties(getLocation(context), + TABLE, + getQualifiedName(context.tableName), + properties, + context.EXISTS() != null); + } + @Override public Node visitRenameColumn(SqlBaseParser.RenameColumnContext context) { diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/AstVisitor.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/AstVisitor.java index 49d3109c984f..8eec8cf4936f 100644 --- a/presto-parser/src/main/java/com/facebook/presto/sql/tree/AstVisitor.java +++ b/presto-parser/src/main/java/com/facebook/presto/sql/tree/AstVisitor.java @@ -127,6 +127,11 @@ protected R visitShowTables(ShowTables node, C context) return visitStatement(node, context); } + protected R visitSetProperties(SetProperties node, C context) + { + return visitStatement(node, context); + } + protected R visitShowSchemas(ShowSchemas node, C context) { return visitStatement(node, context); diff --git a/presto-parser/src/main/java/com/facebook/presto/sql/tree/SetProperties.java b/presto-parser/src/main/java/com/facebook/presto/sql/tree/SetProperties.java new file mode 100644 index 000000000000..ed7d2ef0797e --- /dev/null +++ b/presto-parser/src/main/java/com/facebook/presto/sql/tree/SetProperties.java @@ -0,0 +1,120 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.sql.tree; + +import com.google.common.collect.ImmutableList; + +import java.util.List; +import java.util.Objects; +import java.util.Optional; + +import static com.google.common.base.MoreObjects.toStringHelper; +import static java.util.Objects.requireNonNull; + +public class SetProperties + extends Statement +{ + public enum Type + { + TABLE + } + + private final Type type; + private final QualifiedName tableName; + private final List properties; + private final boolean tableExists; + + public SetProperties(Type type, QualifiedName name, List properties, boolean tableExists) + { + this(Optional.empty(), type, name, properties, tableExists); + } + + public SetProperties(NodeLocation location, Type type, QualifiedName name, List properties, boolean tableExists) + { + this(Optional.of(location), type, name, properties, tableExists); + } + + private SetProperties(Optional location, Type type, QualifiedName name, List properties, boolean tableExists) + { + super(location); + this.type = requireNonNull(type, "type is null"); + this.tableName = requireNonNull(name, "name is null"); + this.properties = ImmutableList.copyOf(requireNonNull(properties, "properties is null")); + this.tableExists = tableExists; + } + + public Type getType() + { + return type; + } + + public boolean isTableExists() + { + return tableExists; + } + + public QualifiedName getTableName() + { + return tableName; + } + + public List getProperties() + { + return properties; + } + + public R accept(AstVisitor visitor, C context) + { + return visitor.visitSetProperties(this, context); + } + + @Override + public List getChildren() + { + return ImmutableList.of(); + } + + @Override + public int hashCode() + { + return Objects.hash(type, tableName, properties, tableExists); + } + + @Override + public boolean equals(Object obj) + { + if (this == obj) { + return true; + } + if ((obj == null) || (getClass() != obj.getClass())) { + return false; + } + SetProperties o = (SetProperties) obj; + return type == o.type && + Objects.equals(tableName, o.tableName) && + Objects.equals(properties, o.properties) && + Objects.equals(tableExists, o.tableExists); + } + + @Override + public String toString() + { + return toStringHelper(this) + .add("type", type) + .add("name", tableName) + .add("properties", properties) + .add("tableExists", tableExists) + .toString(); + } +} diff --git a/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java b/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java index 7b5bba37d60a..ce3715b8c296 100644 --- a/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java +++ b/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java @@ -120,6 +120,7 @@ import com.facebook.presto.sql.tree.Row; import com.facebook.presto.sql.tree.Select; import com.facebook.presto.sql.tree.SelectItem; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetRole; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.ShowCatalogs; @@ -1622,6 +1623,20 @@ public void testRenameTable() assertStatement("ALTER TABLE IF EXISTS a RENAME TO b", new RenameTable(QualifiedName.of("a"), QualifiedName.of("b"), true)); } + @Test + public void testSetProperties() + { + assertStatement("ALTER TABLE a SET PROPERTIES (foo='bar')", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new StringLiteral("bar"))), false)); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=true)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new BooleanLiteral("true"))), false)); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=123)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new LongLiteral("123"))), false)); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=123, bar=456)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new LongLiteral("123")), new Property(new Identifier("bar"), new LongLiteral("456"))), false)); + assertStatement("ALTER TABLE a SET PROPERTIES (\" s p a c e \"='bar')", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier(" s p a c e "), new StringLiteral("bar"))), false)); + + assertInvalidStatement("ALTER TABLE a SET PROPERTIES ()", "mismatched input '\\)'. Expecting: "); + assertStatement("ALTER TABLE IF EXISTS b SET PROPERTIES (foo=12345)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("b"), ImmutableList.of(new Property(new Identifier("foo"), new LongLiteral("12345"))), true)); + assertInvalidStatement("ALTER TABLE IF EXISTS b SET PROPERTIES ()", "mismatched input '\\)'. Expecting: "); + } + @Test public void testRenameColumn() { diff --git a/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java b/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java index edda43375c1a..b2aaac7baf2e 100644 --- a/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java +++ b/presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java @@ -194,6 +194,10 @@ public void testStatementBuilder() printStatement("alter table foo rename to bar"); printStatement("alter table a.b.c rename to d.e.f"); + printStatement("alter table foo set properties (a='1')"); + printStatement("alter table a.b.c set properties (a=true, b=123, c='x')"); + printStatement("alter table if exists bar set properties (b='1')"); + printStatement("alter table a.b.c rename column x to y"); printStatement("alter table a.b.c add column x bigint"); diff --git a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/AllowAllAccessControl.java b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/AllowAllAccessControl.java index 5384a53a3f42..38f94530bedd 100644 --- a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/AllowAllAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/AllowAllAccessControl.java @@ -22,6 +22,7 @@ import com.facebook.presto.spi.security.PrestoPrincipal; import com.facebook.presto.spi.security.Privilege; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -59,6 +60,11 @@ public void checkCanCreateTable(ConnectorTransactionHandle transaction, Connecto { } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanDropTable(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName) { diff --git a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/FileBasedAccessControl.java b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/FileBasedAccessControl.java index f715fd34e606..d9d13ccdb314 100644 --- a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/FileBasedAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/FileBasedAccessControl.java @@ -29,6 +29,7 @@ import java.nio.file.Paths; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -58,6 +59,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denyRenameTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyRevokeTablePrivilege; import static com.facebook.presto.spi.security.AccessDeniedException.denySelectTable; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denyTruncateTable; import static com.facebook.presto.spi.security.AccessDeniedException.denyUpdateTableColumns; @@ -117,6 +119,14 @@ public void checkCanCreateTable(ConnectorTransactionHandle transaction, Connecto } } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + if (!checkTablePermission(identity, tableName, OWNERSHIP)) { + denySetTableProperties(tableName.toString()); + } + } + @Override public void checkCanDropTable(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName) { diff --git a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingConnectorAccessControl.java b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingConnectorAccessControl.java index c6f769f7c9c2..bb4f74134f02 100644 --- a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingConnectorAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingConnectorAccessControl.java @@ -22,6 +22,7 @@ import com.facebook.presto.spi.security.PrestoPrincipal; import com.facebook.presto.spi.security.Privilege; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; @@ -94,6 +95,12 @@ public void checkCanRenameTable(ConnectorTransactionHandle transactionHandle, Co delegate().checkCanRenameTable(transactionHandle, identity, context, tableName, newTableName); } + @Override + public void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + delegate().checkCanSetTableProperties(transactionHandle, identity, context, tableName, properties); + } + @Override public void checkCanShowTablesMetadata(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, String schemaName) { diff --git a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingSystemAccessControl.java b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingSystemAccessControl.java index a280e1a54cd9..0adec4a48aa3 100644 --- a/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingSystemAccessControl.java +++ b/presto-plugin-toolkit/src/main/java/com/facebook/presto/plugin/base/security/ForwardingSystemAccessControl.java @@ -122,6 +122,12 @@ public void checkCanCreateTable(Identity identity, AccessControlContext context, delegate().checkCanCreateTable(identity, context, table); } + @Override + public void checkCanSetTableProperties(Identity identity, AccessControlContext context, CatalogSchemaTableName table) + { + delegate().checkCanSetTableProperties(identity, context, table); + } + @Override public void checkCanDropTable(Identity identity, AccessControlContext context, CatalogSchemaTableName table) { diff --git a/presto-plugin-toolkit/src/test/java/com/facebook/presto/plugin/base/security/TestFileBasedAccessControl.java b/presto-plugin-toolkit/src/test/java/com/facebook/presto/plugin/base/security/TestFileBasedAccessControl.java index 177a0b8ba79d..1c081887df65 100644 --- a/presto-plugin-toolkit/src/test/java/com/facebook/presto/plugin/base/security/TestFileBasedAccessControl.java +++ b/presto-plugin-toolkit/src/test/java/com/facebook/presto/plugin/base/security/TestFileBasedAccessControl.java @@ -23,6 +23,7 @@ import com.facebook.presto.spi.security.AccessControlContext; import com.facebook.presto.spi.security.AccessDeniedException; import com.facebook.presto.spi.security.ConnectorIdentity; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import org.testng.Assert.ThrowingRunnable; import org.testng.annotations.Test; @@ -69,8 +70,11 @@ public void testTableRules() accessControl.checkCanSelectFromColumns(TRANSACTION_HANDLE, user("joe"), CONTEXT, new SchemaTableName("bobschema", "bobtable"), ImmutableSet.of()); accessControl.checkCanCreateViewWithSelectFromColumns(TRANSACTION_HANDLE, user("bob"), CONTEXT, new SchemaTableName("bobschema", "bobtable"), ImmutableSet.of()); accessControl.checkCanDropTable(TRANSACTION_HANDLE, user("admin"), CONTEXT, new SchemaTableName("bobschema", "bobtable")); + accessControl.checkCanSetTableProperties(TRANSACTION_HANDLE, user("admin"), CONTEXT, new SchemaTableName("bobschema", "bobtable"), ImmutableMap.of()); + accessControl.checkCanSetTableProperties(TRANSACTION_HANDLE, user("alice"), CONTEXT, new SchemaTableName("aliceSchema", "aliceTable"), ImmutableMap.of()); assertDenied(() -> accessControl.checkCanInsertIntoTable(TRANSACTION_HANDLE, user("alice"), CONTEXT, new SchemaTableName("bobschema", "bobtable"))); assertDenied(() -> accessControl.checkCanDropTable(TRANSACTION_HANDLE, user("bob"), CONTEXT, new SchemaTableName("bobschema", "bobtable"))); + assertDenied(() -> accessControl.checkCanSetTableProperties(TRANSACTION_HANDLE, user("bob"), CONTEXT, new SchemaTableName("bobschema", "bobtable"), ImmutableMap.of())); assertDenied(() -> accessControl.checkCanInsertIntoTable(TRANSACTION_HANDLE, user("bob"), CONTEXT, new SchemaTableName("test", "test"))); assertDenied(() -> accessControl.checkCanSelectFromColumns(TRANSACTION_HANDLE, user("admin"), CONTEXT, new SchemaTableName("secret", "secret"), ImmutableSet.of())); assertDenied(() -> accessControl.checkCanSelectFromColumns(TRANSACTION_HANDLE, user("joe"), CONTEXT, new SchemaTableName("secret", "secret"), ImmutableSet.of())); diff --git a/presto-plugin-toolkit/src/test/resources/table.json b/presto-plugin-toolkit/src/test/resources/table.json index 1d52a13fd46a..18b76320fc16 100644 --- a/presto-plugin-toolkit/src/test/resources/table.json +++ b/presto-plugin-toolkit/src/test/resources/table.json @@ -15,6 +15,12 @@ "table": "bobtable", "privileges": ["SELECT", "INSERT", "DELETE", "GRANT_SELECT"] }, + { + "user": "alice", + "schema": "aliceschema", + "table": "alicetable", + "privileges": ["OWNERSHIP"] + }, { "privileges": ["SELECT"] } diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java b/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java index 422d34d3fcbf..62f7208301ee 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java @@ -20,6 +20,7 @@ import com.facebook.presto.spi.security.PrestoPrincipal; import com.facebook.presto.spi.security.Privilege; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -48,6 +49,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denySelectColumns; import static com.facebook.presto.spi.security.AccessDeniedException.denySetCatalogSessionProperty; import static com.facebook.presto.spi.security.AccessDeniedException.denySetRole; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowCurrentRoles; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowRoleGrants; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowRoles; @@ -121,6 +123,16 @@ default void checkCanCreateTable(ConnectorTransactionHandle transactionHandle, C denyCreateTable(tableName.toString()); } + /** + * Check if identity is allowed to set properties to the specified table. + * + * @throws com.facebook.presto.spi.security.AccessDeniedException if not allowed + */ + default void checkCanSetTableProperties(ConnectorTransactionHandle transactionHandle, ConnectorIdentity identity, AccessControlContext context, SchemaTableName tableName, Map properties) + { + denySetTableProperties(tableName.toString()); + } + /** * Check if identity is allowed to drop the specified table in this catalog. * diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java b/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java index 313500319bd3..8225d1d129a0 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java @@ -376,6 +376,11 @@ default void renameTable(ConnectorSession session, ConnectorTableHandle tableHan throw new PrestoException(NOT_SUPPORTED, "This connector does not support renaming tables"); } + default void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + throw new PrestoException(NOT_SUPPORTED, "This connector does not support setting table properties"); + } + /** * Add the specified column */ diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java b/presto-spi/src/main/java/com/facebook/presto/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java index 73d696ca6a42..2201e0b98b0b 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/connector/classloader/ClassLoaderSafeConnectorMetadata.java @@ -427,6 +427,13 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand } } + public void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.setTableProperties(session, tableHandle, properties); + } + } + @Override public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata, Optional layout) { diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControl.java b/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControl.java index 0866803040a8..c75f3ee58e24 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControl.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessControl.java @@ -22,6 +22,7 @@ import java.security.Principal; import java.security.cert.X509Certificate; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -113,6 +114,13 @@ default AuthorizedIdentity selectAuthorizedIdentity(Identity identity, AccessCon */ void checkCanRenameTable(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, QualifiedObjectName newTableName); + /** + * Check if identity is allowed to set properties to the specified table. + * + * @throws AccessDeniedException if not allowed + */ + void checkCanSetTableProperties(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, Map properties); + /** * Check if identity is allowed to show metadata of tables by executing SHOW TABLES, SHOW GRANTS etc. in a catalog. *

diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessDeniedException.java b/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessDeniedException.java index 4a8ca11bdf59..1766e1b96199 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessDeniedException.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/security/AccessDeniedException.java @@ -106,6 +106,16 @@ public static void denyCreateTable(String tableName, String extraInfo) throw new AccessDeniedException(format("Cannot create table %s%s", tableName, formatExtraInfo(extraInfo))); } + public static void denySetTableProperties(String tableName) + { + denySetTableProperties(tableName, null); + } + + public static void denySetTableProperties(String tableName, String extraInfo) + { + throw new AccessDeniedException(format("Cannot set table properties to %s%s", tableName, formatExtraInfo(extraInfo))); + } + public static void denyDropTable(String tableName) { denyDropTable(tableName, null); diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/security/AllowAllAccessControl.java b/presto-spi/src/main/java/com/facebook/presto/spi/security/AllowAllAccessControl.java index f1410a6f5717..70458ba83ef0 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/security/AllowAllAccessControl.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/security/AllowAllAccessControl.java @@ -20,6 +20,7 @@ import com.facebook.presto.spi.SchemaTableName; import java.security.Principal; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -88,6 +89,11 @@ public void checkCanRenameTable(TransactionId transactionId, Identity identity, { } + @Override + public void checkCanSetTableProperties(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, Map properties) + { + } + @Override public void checkCanShowTablesMetadata(TransactionId transactionId, Identity identity, AccessControlContext context, CatalogSchemaName schema) { diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/security/DenyAllAccessControl.java b/presto-spi/src/main/java/com/facebook/presto/spi/security/DenyAllAccessControl.java index a8a7fca4c969..0fdbbe5576b6 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/security/DenyAllAccessControl.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/security/DenyAllAccessControl.java @@ -21,6 +21,7 @@ import java.security.Principal; import java.util.Collections; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -52,6 +53,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denySetCatalogSessionProperty; import static com.facebook.presto.spi.security.AccessDeniedException.denySetRole; import static com.facebook.presto.spi.security.AccessDeniedException.denySetSystemSessionProperty; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denySetUser; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowCurrentRoles; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowRoleGrants; @@ -127,6 +129,12 @@ public void checkCanRenameTable(TransactionId transactionId, Identity identity, denyRenameTable(tableName.toString(), newTableName.toString()); } + @Override + public void checkCanSetTableProperties(TransactionId transactionId, Identity identity, AccessControlContext context, QualifiedObjectName tableName, Map properties) + { + denySetTableProperties(tableName.toString()); + } + @Override public void checkCanShowTablesMetadata(TransactionId transactionId, Identity identity, AccessControlContext context, CatalogSchemaName schema) { diff --git a/presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java b/presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java index d51b51763138..7d209ce6b392 100644 --- a/presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java +++ b/presto-spi/src/main/java/com/facebook/presto/spi/security/SystemAccessControl.java @@ -45,6 +45,7 @@ import static com.facebook.presto.spi.security.AccessDeniedException.denyRevokeTablePrivilege; import static com.facebook.presto.spi.security.AccessDeniedException.denySelectColumns; import static com.facebook.presto.spi.security.AccessDeniedException.denySetCatalogSessionProperty; +import static com.facebook.presto.spi.security.AccessDeniedException.denySetTableProperties; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowSchemas; import static com.facebook.presto.spi.security.AccessDeniedException.denyShowTablesMetadata; import static com.facebook.presto.spi.security.AccessDeniedException.denyUpdateTableColumns; @@ -157,6 +158,16 @@ default void checkCanCreateTable(Identity identity, AccessControlContext context denyCreateTable(table.toString()); } + /** + * Check if identity is allowed to alter properties to the specified table in a catalog. + * + * @throws AccessDeniedException if not allowed + */ + default void checkCanSetTableProperties(Identity identity, AccessControlContext context, CatalogSchemaTableName table) + { + denySetTableProperties(table.toString()); + } + /** * Check if identity is allowed to drop the specified table in a catalog. * From bb25355e080af6e38aab2dae47f9745b0328d164 Mon Sep 17 00:00:00 2001 From: pratyakshsharma Date: Fri, 4 Oct 2024 19:41:13 +0530 Subject: [PATCH 2/3] Refactor and create new method in MetadataUtil --- .../facebook/presto/execution/AddColumnTask.java | 6 ++---- .../com/facebook/presto/execution/CallTask.java | 6 +++--- .../execution/CreateMaterializedViewTask.java | 7 ++----- .../presto/execution/CreateSchemaTask.java | 9 ++------- .../presto/execution/CreateTableTask.java | 11 ++++------- .../presto/execution/RenameTableTask.java | 7 +++---- .../presto/execution/ResetSessionTask.java | 6 +++--- .../presto/execution/SetSessionTask.java | 6 +++--- .../com/facebook/presto/execution/UseTask.java | 7 ++----- .../facebook/presto/metadata/MetadataUtil.java | 12 ++++++++++++ .../presto/sql/analyzer/StatementAnalyzer.java | 16 ++++++++++------ .../presto/sql/planner/LogicalPlanner.java | 7 ++----- 12 files changed, 48 insertions(+), 52 deletions(-) diff --git a/presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java b/presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java index a18cb98c142a..8a5fd2cff46b 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java @@ -21,7 +21,6 @@ import com.facebook.presto.spi.ColumnMetadata; import com.facebook.presto.spi.ConnectorId; import com.facebook.presto.spi.MaterializedViewDefinition; -import com.facebook.presto.spi.PrestoException; import com.facebook.presto.spi.TableHandle; import com.facebook.presto.spi.WarningCollector; import com.facebook.presto.spi.security.AccessControl; @@ -39,7 +38,7 @@ import static com.facebook.presto.common.type.TypeSignature.parseTypeSignature; import static com.facebook.presto.common.type.UnknownType.UNKNOWN; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT; import static com.facebook.presto.sql.NodeUtils.mapFromProperties; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.COLUMN_ALREADY_EXISTS; @@ -78,8 +77,7 @@ public ListenableFuture execute(AddColumn statement, TransactionManager trans return immediateFuture(null); } - ConnectorId connectorId = metadata.getCatalogHandle(session, tableName.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + tableName.getCatalogName())); + ConnectorId connectorId = getConnectorIdOrThrow(session, metadata, tableName.getCatalogName()); accessControl.checkCanAddColumns(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), tableName); diff --git a/presto-main/src/main/java/com/facebook/presto/execution/CallTask.java b/presto-main/src/main/java/com/facebook/presto/execution/CallTask.java index 247cec1cdff4..e4701070b96c 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/CallTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/CallTask.java @@ -47,14 +47,15 @@ import java.util.function.Predicate; import static com.facebook.presto.common.type.TypeUtils.writeNativeValue; +import static com.facebook.presto.metadata.MetadataUtil.catalogError; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; import static com.facebook.presto.spi.StandardErrorCode.INVALID_PROCEDURE_ARGUMENT; import static com.facebook.presto.spi.StandardErrorCode.INVALID_PROCEDURE_DEFINITION; import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED; import static com.facebook.presto.spi.StandardErrorCode.PROCEDURE_CALL_FAILED; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.INVALID_PROCEDURE_ARGUMENTS; -import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor; import static com.facebook.presto.sql.planner.ExpressionInterpreter.evaluateConstantExpression; import static com.facebook.presto.util.Failures.checkCondition; @@ -81,8 +82,7 @@ public ListenableFuture execute(Call call, TransactionManager transactionMana } QualifiedObjectName procedureName = createQualifiedObjectName(session, call, call.getName()); - ConnectorId connectorId = metadata.getCatalogHandle(session, procedureName.getCatalogName()) - .orElseThrow(() -> new SemanticException(MISSING_CATALOG, call, "Catalog %s does not exist", procedureName.getCatalogName())); + ConnectorId connectorId = getConnectorIdOrThrow(session, metadata, procedureName.getCatalogName(), call, catalogError); Procedure procedure = metadata.getProcedureRegistry().resolve(connectorId, toSchemaTableName(procedureName)); // map declared argument names to positions diff --git a/presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java b/presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java index e80c786b6e48..1f07b9088679 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java @@ -17,7 +17,6 @@ import com.facebook.presto.common.QualifiedObjectName; import com.facebook.presto.metadata.Metadata; import com.facebook.presto.spi.ColumnMetadata; -import com.facebook.presto.spi.ConnectorId; import com.facebook.presto.spi.ConnectorTableMetadata; import com.facebook.presto.spi.MaterializedViewDefinition; import com.facebook.presto.spi.PrestoException; @@ -44,9 +43,9 @@ import java.util.Optional; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; import static com.facebook.presto.sql.NodeUtils.mapFromProperties; import static com.facebook.presto.sql.SqlFormatterUtil.getFormattedSql; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MATERIALIZED_VIEW_ALREADY_EXISTS; @@ -93,8 +92,6 @@ public ListenableFuture execute(CreateMaterializedView statement, Transaction Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector); Analysis analysis = analyzer.analyze(statement); - ConnectorId connectorId = metadata.getCatalogHandle(session, viewName.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + viewName.getCatalogName())); List columnMetadata = analysis.getOutputDescriptor(statement.getQuery()) .getVisibleFields().stream() .map(field -> new ColumnMetadata(field.getName().get(), field.getType())) @@ -102,7 +99,7 @@ public ListenableFuture execute(CreateMaterializedView statement, Transaction Map sqlProperties = mapFromProperties(statement.getProperties()); Map properties = metadata.getTablePropertyManager().getProperties( - connectorId, + getConnectorIdOrThrow(session, metadata, viewName.getCatalogName()), viewName.getCatalogName(), sqlProperties, session, diff --git a/presto-main/src/main/java/com/facebook/presto/execution/CreateSchemaTask.java b/presto-main/src/main/java/com/facebook/presto/execution/CreateSchemaTask.java index f7b15d470933..1b04545b5874 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/CreateSchemaTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/CreateSchemaTask.java @@ -16,8 +16,6 @@ import com.facebook.presto.Session; import com.facebook.presto.common.CatalogSchemaName; import com.facebook.presto.metadata.Metadata; -import com.facebook.presto.spi.ConnectorId; -import com.facebook.presto.spi.PrestoException; import com.facebook.presto.spi.WarningCollector; import com.facebook.presto.spi.security.AccessControl; import com.facebook.presto.sql.analyzer.SemanticException; @@ -31,7 +29,7 @@ import java.util.Optional; import static com.facebook.presto.metadata.MetadataUtil.createCatalogSchemaName; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.sql.NodeUtils.mapFromProperties; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.SCHEMA_ALREADY_EXISTS; import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor; @@ -68,11 +66,8 @@ public ListenableFuture execute(CreateSchema statement, TransactionManager tr return immediateFuture(null); } - ConnectorId connectorId = metadata.getCatalogHandle(session, schema.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + schema.getCatalogName())); - Map properties = metadata.getSchemaPropertyManager().getProperties( - connectorId, + getConnectorIdOrThrow(session, metadata, schema.getCatalogName()), schema.getCatalogName(), mapFromProperties(statement.getProperties()), session, diff --git a/presto-main/src/main/java/com/facebook/presto/execution/CreateTableTask.java b/presto-main/src/main/java/com/facebook/presto/execution/CreateTableTask.java index 09657edcc9ff..719ff812ba8f 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/CreateTableTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/CreateTableTask.java @@ -57,15 +57,15 @@ import static com.facebook.presto.common.type.UnknownType.UNKNOWN; import static com.facebook.presto.execution.AddConstraintTask.convertToTableConstraint; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; +import static com.facebook.presto.metadata.MetadataUtil.likeTableCatalogError; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS; import static com.facebook.presto.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; import static com.facebook.presto.spi.StandardErrorCode.SYNTAX_ERROR; import static com.facebook.presto.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT; import static com.facebook.presto.sql.NodeUtils.mapFromProperties; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.DUPLICATE_COLUMN_NAME; -import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_TABLE; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.TABLE_ALREADY_EXISTS; @@ -111,8 +111,7 @@ public ListenableFuture internalExecute(CreateTable statement, Metadata metad return immediateFuture(null); } - ConnectorId connectorId = metadata.getCatalogHandle(session, tableName.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + tableName.getCatalogName())); + ConnectorId connectorId = getConnectorIdOrThrow(session, metadata, tableName.getCatalogName()); LinkedHashMap columns = new LinkedHashMap<>(); Map inheritedProperties = ImmutableMap.of(); @@ -159,9 +158,7 @@ public ListenableFuture internalExecute(CreateTable statement, Metadata metad else if (element instanceof LikeClause) { LikeClause likeClause = (LikeClause) element; QualifiedObjectName likeTableName = createQualifiedObjectName(session, statement, likeClause.getTableName()); - if (!metadata.getCatalogHandle(session, likeTableName.getCatalogName()).isPresent()) { - throw new SemanticException(MISSING_CATALOG, statement, "LIKE table catalog '%s' does not exist", likeTableName.getCatalogName()); - } + getConnectorIdOrThrow(session, metadata, likeTableName.getCatalogName(), statement, likeTableCatalogError); if (!tableName.getCatalogName().equals(likeTableName.getCatalogName())) { throw new SemanticException(NOT_SUPPORTED, statement, "LIKE table across catalogs is not supported"); } diff --git a/presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java b/presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java index 10b662632436..99d318d39bab 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java @@ -30,7 +30,8 @@ import java.util.Optional; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; -import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; +import static com.facebook.presto.metadata.MetadataUtil.targetTableCatalogError; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_TABLE; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.TABLE_ALREADY_EXISTS; @@ -66,9 +67,7 @@ public ListenableFuture execute(RenameTable statement, TransactionManager tra } QualifiedObjectName target = createQualifiedObjectName(session, statement, statement.getTarget()); - if (!metadata.getCatalogHandle(session, target.getCatalogName()).isPresent()) { - throw new SemanticException(MISSING_CATALOG, statement, "Target catalog '%s' does not exist", target.getCatalogName()); - } + getConnectorIdOrThrow(session, metadata, target.getCatalogName(), statement, targetTableCatalogError); if (metadata.getMetadataResolver(session).getTableHandle(target).isPresent()) { throw new SemanticException(TABLE_ALREADY_EXISTS, statement, "Target table '%s' already exists", target); } diff --git a/presto-main/src/main/java/com/facebook/presto/execution/ResetSessionTask.java b/presto-main/src/main/java/com/facebook/presto/execution/ResetSessionTask.java index 2a0825f83527..b26641ecef57 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/ResetSessionTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/ResetSessionTask.java @@ -24,8 +24,9 @@ import java.util.List; +import static com.facebook.presto.metadata.MetadataUtil.catalogError; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.INVALID_SESSION_PROPERTY; -import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; import static com.google.common.util.concurrent.Futures.immediateFuture; public class ResetSessionTask @@ -51,8 +52,7 @@ public ListenableFuture execute(ResetSession statement, TransactionManager tr .orElseThrow(() -> new SemanticException(INVALID_SESSION_PROPERTY, statement, "Session property %s does not exist", statement.getName())); } else { - ConnectorId connectorId = metadata.getCatalogHandle(stateMachine.getSession(), parts.get(0)) - .orElseThrow(() -> new SemanticException(MISSING_CATALOG, statement, "Catalog %s does not exist", parts.get(0))); + ConnectorId connectorId = getConnectorIdOrThrow(stateMachine.getSession(), metadata, parts.get(0), statement, catalogError); metadata.getSessionPropertyManager().getConnectorSessionPropertyMetadata(connectorId, parts.get(1)) .orElseThrow(() -> new SemanticException(INVALID_SESSION_PROPERTY, statement, "Session property %s does not exist", statement.getName())); } diff --git a/presto-main/src/main/java/com/facebook/presto/execution/SetSessionTask.java b/presto-main/src/main/java/com/facebook/presto/execution/SetSessionTask.java index e96acf2698f5..f1f8bbc67cf3 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/SetSessionTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/SetSessionTask.java @@ -30,10 +30,11 @@ import java.util.List; +import static com.facebook.presto.metadata.MetadataUtil.catalogError; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.metadata.SessionPropertyManager.evaluatePropertyValue; import static com.facebook.presto.metadata.SessionPropertyManager.serializeSessionProperty; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.INVALID_SESSION_PROPERTY; -import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor; import static com.google.common.util.concurrent.Futures.immediateFuture; import static java.lang.String.format; @@ -65,8 +66,7 @@ public ListenableFuture execute(SetSession statement, TransactionManager tran .orElseThrow(() -> new SemanticException(INVALID_SESSION_PROPERTY, statement, "Session property %s does not exist", statement.getName())); } else { - ConnectorId connectorId = metadata.getCatalogHandle(stateMachine.getSession(), parts.get(0)) - .orElseThrow(() -> new SemanticException(MISSING_CATALOG, statement, "Catalog %s does not exist", parts.get(0))); + ConnectorId connectorId = getConnectorIdOrThrow(stateMachine.getSession(), metadata, parts.get(0), statement, catalogError); accessControl.checkCanSetCatalogSessionProperty(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), parts.get(0), parts.get(1)); propertyMetadata = metadata.getSessionPropertyManager().getConnectorSessionPropertyMetadata(connectorId, parts.get(1)) .orElseThrow(() -> new SemanticException(INVALID_SESSION_PROPERTY, statement, "Session property %s does not exist", statement.getName())); diff --git a/presto-main/src/main/java/com/facebook/presto/execution/UseTask.java b/presto-main/src/main/java/com/facebook/presto/execution/UseTask.java index cf3925581455..f677165a97f6 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/UseTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/UseTask.java @@ -15,7 +15,6 @@ import com.facebook.presto.Session; import com.facebook.presto.metadata.Metadata; -import com.facebook.presto.spi.PrestoException; import com.facebook.presto.spi.security.AccessControl; import com.facebook.presto.sql.analyzer.SemanticException; import com.facebook.presto.sql.tree.Expression; @@ -25,7 +24,7 @@ import java.util.List; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.CATALOG_NOT_SPECIFIED; import static com.google.common.util.concurrent.Futures.immediateFuture; @@ -69,9 +68,7 @@ private void checkAndSetCatalog(Use statement, Metadata metadata, QueryStateMach { if (statement.getCatalog().isPresent()) { String catalog = statement.getCatalog().get().getValueLowerCase(); - if (!metadata.getCatalogHandle(session, catalog).isPresent()) { - throw new PrestoException(NOT_FOUND, "Catalog does not exist: " + catalog); - } + getConnectorIdOrThrow(session, metadata, catalog); stateMachine.setSetCatalog(catalog); } } diff --git a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java index b00783903ad9..6e13a37eccbf 100644 --- a/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java +++ b/presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java @@ -32,6 +32,7 @@ import com.facebook.presto.sql.tree.Node; import com.facebook.presto.sql.tree.PrincipalSpecification; import com.facebook.presto.sql.tree.QualifiedName; +import com.facebook.presto.sql.tree.Statement; import com.facebook.presto.transaction.TransactionManager; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -46,6 +47,7 @@ import static com.facebook.presto.spi.security.PrincipalType.USER; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.CATALOG_NOT_SPECIFIED; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.INVALID_SCHEMA_NAME; +import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG; import static com.facebook.presto.sql.analyzer.SemanticErrorCode.SCHEMA_NOT_SPECIFIED; import static com.google.common.base.Preconditions.checkArgument; import static java.lang.String.format; @@ -56,6 +58,10 @@ public final class MetadataUtil { private MetadataUtil() {} + public static final String likeTableCatalogError = "LIKE table catalog '%s' does not exist"; + public static final String catalogError = "Catalog %s does not exist"; + public static final String targetTableCatalogError = "Target catalog '%s' does not exist"; + public static void checkTableName(String catalogName, Optional schemaName, Optional tableName) { checkCatalogName(catalogName); @@ -91,6 +97,12 @@ public static ConnectorId getConnectorIdOrThrow(Session session, Metadata metada .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + catalogName)); } + public static ConnectorId getConnectorIdOrThrow(Session session, Metadata metadata, String catalogName, Statement statement, String errorMsg) + { + return metadata.getCatalogHandle(session, catalogName) + .orElseThrow(() -> new SemanticException(MISSING_CATALOG, statement, errorMsg, catalogName)); + } + public static String checkLowerCase(String value, String name) { if (value == null) { diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java index 0bd1e9030404..d9fb6707c20d 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java @@ -35,7 +35,6 @@ import com.facebook.presto.metadata.OperatorNotFoundException; import com.facebook.presto.spi.ColumnHandle; import com.facebook.presto.spi.ColumnMetadata; -import com.facebook.presto.spi.ConnectorId; import com.facebook.presto.spi.MaterializedViewDefinition; import com.facebook.presto.spi.MaterializedViewStatus; import com.facebook.presto.spi.PrestoException; @@ -147,6 +146,7 @@ import com.facebook.presto.sql.tree.Select; import com.facebook.presto.sql.tree.SelectItem; import com.facebook.presto.sql.tree.SetOperation; +import com.facebook.presto.sql.tree.SetProperties; import com.facebook.presto.sql.tree.SetSession; import com.facebook.presto.sql.tree.SimpleGroupBy; import com.facebook.presto.sql.tree.SingleColumn; @@ -202,10 +202,10 @@ import static com.facebook.presto.common.type.UnknownType.UNKNOWN; import static com.facebook.presto.common.type.VarcharType.VARCHAR; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; import static com.facebook.presto.spi.StandardErrorCode.INVALID_ARGUMENTS; import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; import static com.facebook.presto.spi.StandardWarningCode.PERFORMANCE_WARNING; import static com.facebook.presto.spi.StandardWarningCode.REDUNDANT_ORDER_BY; import static com.facebook.presto.spi.analyzer.AccessControlRole.TABLE_CREATE; @@ -615,12 +615,10 @@ protected Scope visitAnalyze(Analyze node, Optional scope) } validateProperties(node.getProperties(), scope); - ConnectorId connectorId = metadata.getCatalogHandle(session, tableName.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog not found: " + tableName.getCatalogName())); Map analyzeProperties = metadata.getAnalyzePropertyManager().getProperties( - connectorId, - connectorId.getCatalogName(), + getConnectorIdOrThrow(session, metadata, tableName.getCatalogName()), + tableName.getCatalogName(), mapFromProperties(node.getProperties()), session, metadata, @@ -972,6 +970,12 @@ protected Scope visitRenameTable(RenameTable node, Optional scope) return createAndAssignScope(node, scope); } + @Override + protected Scope visitSetProperties(SetProperties node, Optional scope) + { + return createAndAssignScope(node, scope); + } + @Override protected Scope visitRenameColumn(RenameColumn node, Optional scope) { diff --git a/presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java b/presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java index 8c5a5c7a52a0..660437639939 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/planner/LogicalPlanner.java @@ -88,8 +88,8 @@ import static com.facebook.presto.common.type.BigintType.BIGINT; import static com.facebook.presto.common.type.VarbinaryType.VARBINARY; +import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; -import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND; import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED; import static com.facebook.presto.spi.plan.AggregationNode.singleGroupingSet; import static com.facebook.presto.spi.plan.LimitNode.Step.FINAL; @@ -572,11 +572,8 @@ private RelationPlan createRelationPlan(Analysis analysis, Query query, SqlPlann private ConnectorTableMetadata createTableMetadata(QualifiedObjectName table, List columns, Map propertyExpressions, Map, Expression> parameters, Optional comment) { - ConnectorId connectorId = metadata.getCatalogHandle(session, table.getCatalogName()) - .orElseThrow(() -> new PrestoException(NOT_FOUND, "Catalog does not exist: " + table.getCatalogName())); - Map properties = metadata.getTablePropertyManager().getProperties( - connectorId, + getConnectorIdOrThrow(session, metadata, table.getCatalogName()), table.getCatalogName(), propertyExpressions, session, From 4f826af6fbf7977c5d4171330527a819fb90d17b Mon Sep 17 00:00:00 2001 From: pratyakshsharma Date: Fri, 4 Oct 2024 19:42:13 +0530 Subject: [PATCH 3/3] Add set properties support for iceberg --- .../src/main/sphinx/connector/iceberg.rst | 10 ++++- .../iceberg/IcebergAbstractMetadata.java | 25 +++++++++++ .../IcebergDistributedSmokeTestBase.java | 43 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) diff --git a/presto-docs/src/main/sphinx/connector/iceberg.rst b/presto-docs/src/main/sphinx/connector/iceberg.rst index a8c3c7d362b0..86f83bb3674c 100644 --- a/presto-docs/src/main/sphinx/connector/iceberg.rst +++ b/presto-docs/src/main/sphinx/connector/iceberg.rst @@ -1051,7 +1051,7 @@ that is stored using the ORC file format, partitioned by ``ds`` and partitioning = ARRAY['ds', 'country'] ) -Create an Iceberg table with Iceberg format version 2:: +Create an Iceberg table with Iceberg format version 2 and with commit_retries set to 5:: CREATE TABLE iceberg.web.page_views_v2 ( view_time timestamp, @@ -1063,7 +1063,8 @@ Create an Iceberg table with Iceberg format version 2:: WITH ( format = 'ORC', partitioning = ARRAY['ds', 'country'], - format_version = '2' + format_version = '2', + commit_retries = 5 ) Partition Column Transform @@ -1203,6 +1204,11 @@ The table is partitioned by the transformed value of the column:: ALTER TABLE iceberg.web.page_views ADD COLUMN ts timestamp WITH (partitioning = 'hour'); +Table properties can be modified for an Iceberg table using an ALTER TABLE SET PROPERTIES statement. Only `commit_retries` can be modified at present. +For example, to set `commit_retries` to 6 for the table `iceberg.web.page_views_v2`, use:: + + ALTER TABLE iceberg.web.page_views_v2 SET PROPERTIES (commit_retries = 6); + TRUNCATE ^^^^^^^^ diff --git a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java index 113da1c71d54..60bb0e8e4ffb 100644 --- a/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java +++ b/presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java @@ -84,7 +84,9 @@ import org.apache.iceberg.Schema; import org.apache.iceberg.SchemaParser; import org.apache.iceberg.Table; +import org.apache.iceberg.TableProperties; import org.apache.iceberg.Transaction; +import org.apache.iceberg.UpdateProperties; import org.apache.iceberg.exceptions.NoSuchTableException; import org.apache.iceberg.types.Type; import org.apache.iceberg.types.TypeUtil; @@ -122,6 +124,7 @@ import static com.facebook.presto.iceberg.IcebergPartitionType.ALL; import static com.facebook.presto.iceberg.IcebergSessionProperties.getCompressionCodec; import static com.facebook.presto.iceberg.IcebergSessionProperties.isPushdownFilterEnabled; +import static com.facebook.presto.iceberg.IcebergTableProperties.COMMIT_RETRIES; import static com.facebook.presto.iceberg.IcebergTableProperties.DELETE_MODE; import static com.facebook.presto.iceberg.IcebergTableProperties.FILE_FORMAT_PROPERTY; import static com.facebook.presto.iceberg.IcebergTableProperties.FORMAT_VERSION; @@ -990,6 +993,28 @@ public OptionalLong metadataDelete(ConnectorSession session, ConnectorTableHandl return removeScanFiles(icebergTable, domainPredicate); } + @Override + public void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + IcebergTableHandle handle = (IcebergTableHandle) tableHandle; + Table icebergTable = getIcebergTable(session, handle.getSchemaTableName()); + transaction = icebergTable.newTransaction(); + + UpdateProperties updateProperties = transaction.updateProperties(); + for (Map.Entry entry : properties.entrySet()) { + switch (entry.getKey()) { + case COMMIT_RETRIES: + updateProperties.set(TableProperties.COMMIT_NUM_RETRIES, String.valueOf(entry.getValue())); + break; + default: + throw new PrestoException(NOT_SUPPORTED, "Updating property " + entry.getKey() + " is not supported currently"); + } + } + + updateProperties.commit(); + transaction.commitTransaction(); + } + /** * Deletes all the files for a specific predicate * diff --git a/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java b/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java index 98bfef4aab87..57a8acfae41a 100644 --- a/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java +++ b/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java @@ -52,6 +52,7 @@ import static java.util.stream.Collectors.joining; import static java.util.stream.IntStream.range; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; @@ -1808,4 +1809,46 @@ private Session sessionForTimezone(String zoneId, boolean legacyTimestamp) } return sessionBuilder.build(); } + + @Test + public void testUpdatingInvalidProperty() + { + Session session = getSession(); + String tableName = "test_invalid_property_update"; + assertUpdate(session, "CREATE TABLE " + tableName + " (c1 integer, c2 varchar) WITH(commit_retries = 4)"); + assertThatThrownBy(() -> assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES (format = 'PARQUET')")) + .hasMessage("Updating property format is not supported currently"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + public void testUpdatingRandomProperty() + { + Session session = getSession(); + String tableName = "test_random_property_update"; + assertUpdate(session, "CREATE TABLE " + tableName + " (c1 integer, c2 varchar) WITH(commit_retries = 4)"); + assertThatThrownBy(() -> assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES (some_config = 2)")) + .hasMessage("Catalog 'iceberg' does not support table property 'some_config'"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + public void testUpdatingCommitRetries() + { + Session session = getSession(); + String tableName = "test_commit_retries_update"; + assertUpdate(session, "CREATE TABLE " + tableName + " (c1 integer, c2 varchar) WITH(commit_retries = 4)"); + assertQuery("SELECT value FROM \"" + tableName + "$properties\" WHERE key = 'commit.retry.num-retries'", "VALUES 4"); + assertUpdate("ALTER TABLE " + tableName + " SET PROPERTIES (commit_retries = 5)"); + assertUpdate("ALTER TABLE IF EXISTS " + tableName + " SET PROPERTIES (commit_retries = 6)"); + assertQuery("SELECT value FROM \"" + tableName + "$properties\" WHERE key = 'commit.retry.num-retries'", "VALUES 6"); + assertUpdate("DROP TABLE " + tableName); + } + + @Test + public void testUpdateNonExistentTable() + { + assertQuerySucceeds("ALTER TABLE IF EXISTS non_existent_test_table1 SET PROPERTIES (commit_retries = 6)"); + assertQueryFails("ALTER TABLE non_existent_test_table2 SET PROPERTIES (commit_retries = 6)", "Table does not exist: iceberg.tpch.non_existent_test_table2"); + } }