trinodb · ebyhr · Oct 26, 2021
diff --git a/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java b/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java
@@ -33,7 +33,6 @@
 import io.trino.spi.security.AccessDeniedException;
 import io.trino.spi.type.Type;
 import io.trino.spi.type.TypeNotFoundException;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.analyzer.Output;
 import io.trino.sql.analyzer.OutputColumn;
 import io.trino.sql.tree.ColumnDefinition;
@@ -45,8 +44,6 @@
 import io.trino.sql.tree.TableElement;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -84,14 +81,6 @@
 public class CreateTableTask
         implements DataDefinitionTask<CreateTable>
 {
-    private final boolean disableSetPropertiesSecurityCheckForCreateDdl;
-
-    @Inject
-    public CreateTableTask(FeaturesConfig featuresConfig)
-    {
-        this.disableSetPropertiesSecurityCheckForCreateDdl = featuresConfig.isDisableSetPropertiesSecurityCheckForCreateDdl();
-    }
-
     @Override
     public String getName()
     {
@@ -253,12 +242,7 @@ else if (element instanceof LikeClause) {
                 parameterLookup,
                 true);
 
-        if (!disableSetPropertiesSecurityCheckForCreateDdl && !properties.isEmpty()) {
-            accessControl.checkCanCreateTable(session.toSecurityContext(), tableName, properties);
-        }
-        else {
-            accessControl.checkCanCreateTable(session.toSecurityContext(), tableName);
-        }
+        accessControl.checkCanCreateTable(session.toSecurityContext(), tableName);
 
         Map<String, Object> finalProperties = combineProperties(sqlProperties.keySet(), properties, inheritedProperties);
 

diff --git a/core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java b/core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java
@@ -56,6 +56,9 @@ public ListenableFuture<Void> execute(
             List<Expression> parameters,
             WarningCollector warningCollector)
     {
+        // Disable until we fix some issues (nested properties, reset values to their default, etc)
+        throwUnsupportedError(statement);
+
         Session session = stateMachine.getSession();
         QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
 
@@ -80,6 +83,11 @@ public ListenableFuture<Void> execute(
         return immediateVoidFuture();
     }
 
+    private void throwUnsupportedError(SetProperties statement)
+    {
+        throw semanticException(NOT_SUPPORTED, statement, "Unsupported statement");
+    }
+
     private void setTableProperties(SetProperties statement, QualifiedObjectName tableName, Metadata metadata, AccessControl accessControl, Session session, Map<String, Object> properties)
     {
         if (metadata.getMaterializedView(session, tableName).isPresent()) {

diff --git a/core/trino-main/src/main/java/io/trino/security/AccessControl.java b/core/trino-main/src/main/java/io/trino/security/AccessControl.java
@@ -164,9 +164,7 @@ public interface AccessControl
      * Check if identity is allowed to create the specified table.
      *
      * @throws AccessDeniedException if not allowed
-     * @deprecated use {@link #checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties)}
      */
-    @Deprecated
     void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName);
 
     /**

diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java
@@ -141,7 +141,6 @@ public class FeaturesConfig
     private int maxGroupingSets = 2048;
 
     private boolean legacyCatalogRoles;
-    private boolean disableSetPropertiesSecurityCheckForCreateDdl;
 
     public enum JoinReorderingStrategy
     {
@@ -1091,16 +1090,4 @@ public FeaturesConfig setLegacyCatalogRoles(boolean legacyCatalogRoles)
         this.legacyCatalogRoles = legacyCatalogRoles;
         return this;
     }
-
-    public boolean isDisableSetPropertiesSecurityCheckForCreateDdl()
-    {
-        return disableSetPropertiesSecurityCheckForCreateDdl;
-    }
-
-    @Config("deprecated.disable-set-properties-security-check-for-create-ddl")
-    public FeaturesConfig setDisableSetPropertiesSecurityCheckForCreateDdl(boolean disableSetPropertiesSecurityCheckForCreateDdl)
-    {
-        this.disableSetPropertiesSecurityCheckForCreateDdl = disableSetPropertiesSecurityCheckForCreateDdl;
-        return this;
-    }
 }
diff --git a/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java b/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java
@@ -457,7 +457,7 @@ private LocalQueryRunner(
                 transactionId.map(tId -> transactionManager.getTransactionInfo(tId).isAutoCommitContext()));
 
         dataDefinitionTask = ImmutableMap.<Class<? extends Statement>, DataDefinitionTask<?>>builder()
-                .put(CreateTable.class, new CreateTableTask(featuresConfig))
+                .put(CreateTable.class, new CreateTableTask())
                 .put(CreateView.class, new CreateViewTask(sqlParser, groupProvider, statsCalculator))
                 .put(DropTable.class, new DropTableTask())
                 .put(DropView.class, new DropViewTask())

diff --git a/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java b/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java
@@ -40,7 +40,6 @@
 import io.trino.spi.type.Type;
 import io.trino.spi.type.TypeId;
 import io.trino.spi.type.TypeSignature;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.planner.TestingConnectorTransactionHandle;
 import io.trino.sql.tree.ColumnDefinition;
 import io.trino.sql.tree.CreateTable;
@@ -145,7 +144,7 @@ public void testCreateTableNotExistsTrue()
                 ImmutableList.of(),
                 Optional.empty());
 
-        getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}));
+        getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}));
         assertEquals(metadata.getCreateTableCallCount(), 1);
     }
 
@@ -158,7 +157,7 @@ public void testCreateTableNotExistsFalse()
                 ImmutableList.of(),
                 Optional.empty());
 
-        assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
+        assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
                 .hasErrorCode(ALREADY_EXISTS)
                 .hasMessage("Table already exists");
 
@@ -174,7 +173,7 @@ public void testCreateTableWithMaterializedViewPropertyFails()
                 ImmutableList.of(new Property(new Identifier("foo"), new StringLiteral("bar"))),
                 Optional.empty());
 
-        assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
+        assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
                 .hasErrorCode(INVALID_TABLE_PROPERTY)
                 .hasMessage("Catalog 'catalog' does not support table property 'foo'");
 
@@ -191,7 +190,7 @@ public void testCreateWithNotNullColumns()
                 new ColumnDefinition(identifier("c"), toSqlType(VARBINARY), false, emptyList(), Optional.empty()));
         CreateTable statement = new CreateTable(QualifiedName.of("test_table"), inputColumns, true, ImmutableList.of(), Optional.empty());
 
-        getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}));
+        getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}));
         assertEquals(metadata.getCreateTableCallCount(), 1);
         List<ColumnMetadata> columns = metadata.getReceivedTableMetadata().get(0).getColumns();
         assertEquals(columns.size(), 3);
@@ -224,7 +223,7 @@ public void testCreateWithUnsupportedConnectorThrowsWhenNotNull()
                 Optional.empty());
 
         assertTrinoExceptionThrownBy(() ->
-                getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
+                getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})))
                 .hasErrorCode(NOT_SUPPORTED)
                 .hasMessage("Catalog 'catalog' does not support non-null column for column name 'b'");
     }
@@ -234,7 +233,7 @@ public void testCreateLike()
     {
         CreateTable statement = getCreatleLikeStatement(false);
 
-        getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {}));
+        getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {}));
         assertEquals(metadata.getCreateTableCallCount(), 1);
 
         assertThat(metadata.getReceivedTableMetadata().get(0).getColumns())
@@ -247,7 +246,7 @@ public void testCreateLikeWithProperties()
     {
         CreateTable statement = getCreatleLikeStatement(true);
 
-        getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {}));
+        getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {}));
         assertEquals(metadata.getCreateTableCallCount(), 1);
 
         assertThat(metadata.getReceivedTableMetadata().get(0).getColumns())
@@ -264,7 +263,7 @@ public void testCreateLikeDenyPermission()
         TestingAccessControlManager accessControl = new TestingAccessControlManager(transactionManager, new EventListenerManager(new EventListenerConfig()));
         accessControl.deny(privilege("parent_table", SELECT_COLUMN));
 
-        assertThatThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {})))
+        assertThatThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {})))
                 .isInstanceOf(AccessDeniedException.class)
                 .hasMessageContaining("Cannot reference columns of table");
     }
@@ -277,7 +276,7 @@ public void testCreateLikeWithPropertiesDenyPermission()
         TestingAccessControlManager accessControl = new TestingAccessControlManager(transactionManager, new EventListenerManager(new EventListenerConfig()));
         accessControl.deny(privilege("parent_table", SHOW_CREATE_TABLE));
 
-        assertThatThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {})))
+        assertThatThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {})))
                 .isInstanceOf(AccessDeniedException.class)
                 .hasMessageContaining("Cannot reference properties of table");
     }

diff --git a/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java b/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java
@@ -113,8 +113,7 @@ public void testDefaults()
                 .setUseTableScanNodePartitioning(true)
                 .setTableScanNodePartitioningMinBucketToTaskRatio(0.5)
                 .setMergeProjectWithValues(true)
-                .setLegacyCatalogRoles(false)
-                .setDisableSetPropertiesSecurityCheckForCreateDdl(false));
+                .setLegacyCatalogRoles(false));
     }
 
     @Test
@@ -193,7 +192,6 @@ public void testExplicitPropertyMappings()
                 .put("optimizer.table-scan-node-partitioning-min-bucket-to-task-ratio", "0.0")
                 .put("optimizer.merge-project-with-values", "false")
                 .put("deprecated.legacy-catalog-roles", "true")
-                .put("deprecated.disable-set-properties-security-check-for-create-ddl", "true")
                 .build();
 
         FeaturesConfig expected = new FeaturesConfig()
@@ -268,8 +266,7 @@ public void testExplicitPropertyMappings()
                 .setUseTableScanNodePartitioning(false)
                 .setTableScanNodePartitioningMinBucketToTaskRatio(0.0)
                 .setMergeProjectWithValues(false)
-                .setLegacyCatalogRoles(true)
-                .setDisableSetPropertiesSecurityCheckForCreateDdl(true);
+                .setLegacyCatalogRoles(true);
         assertFullMapping(properties, expected);
     }
 }
diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java
@@ -160,9 +160,7 @@ default void checkCanShowCreateTable(ConnectorSecurityContext context, SchemaTab
      * Check if identity is allowed to create the specified table.
      *
      * @throws io.trino.spi.security.AccessDeniedException if not allowed
-     * @deprecated use {@link #checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties)} instead
      */
-    @Deprecated
     default void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName)
     {
         denyCreateTable(tableName.toString());

diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java
@@ -286,9 +286,7 @@ default void checkCanShowCreateTable(SystemSecurityContext context, CatalogSchem
      * Check if identity is allowed to create the specified table in a catalog.
      *
      * @throws AccessDeniedException if not allowed
-     * @deprecated use {@link #checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties)} instead
      */
-    @Deprecated
     default void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table)
     {
         denyCreateTable(table.toString());

diff --git a/docs/src/main/sphinx/sql/alter-table.rst b/docs/src/main/sphinx/sql/alter-table.rst
@@ -14,7 +14,6 @@ Synopsis
     ALTER TABLE [ IF EXISTS ] name DROP COLUMN [ IF EXISTS ] column_name
     ALTER TABLE [ IF EXISTS ] name RENAME COLUMN [ IF EXISTS ] old_name TO new_name
     ALTER TABLE name SET AUTHORIZATION ( user | USER user | ROLE role )
-    ALTER TABLE name SET PROPERTIES property_name = expression [, ...]
     ALTER TABLE name EXECUTE command [ ( parameter => expression [, ... ] ) ]
         [ WHERE expression ]
 
@@ -82,10 +81,6 @@ Allow everyone with role public to drop and alter table ``people``::
 
     ALTER TABLE people SET AUTHORIZATION ROLE PUBLIC
 
-Set table properties (``x=y``) to table ``users``::
-
-    ALTER TABLE people SET PROPERTIES x = 'y'
-
 Collapse files in a table that are over 10 megabytes in size, as supported by
 the Hive connector::
 

diff --git a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
@@ -66,9 +66,7 @@
 import java.util.UUID;
 import java.util.function.BiFunction;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Strings.isNullOrEmpty;
-import static io.trino.plugin.clickhouse.ClickHouseTableProperties.SAMPLE_BY_PROPERTY;
 import static io.trino.plugin.jdbc.DecimalConfig.DecimalMapping.ALLOW_OVERFLOW;
 import static io.trino.plugin.jdbc.DecimalSessionSessionProperties.getDecimalDefaultScale;
 import static io.trino.plugin.jdbc.DecimalSessionSessionProperties.getDecimalRounding;
@@ -216,27 +214,6 @@ protected String createTableSql(RemoteTableName remoteTableName, List<String> co
         return format("CREATE TABLE %s (%s) %s", quoted(remoteTableName), join(", ", columns), join(" ", tableOptions.build()));
     }
 
-    @Override
-    public void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map<String, Object> properties)
-    {
-        // TODO: Support other table properties
-        checkArgument(properties.size() == 1 && properties.containsKey(SAMPLE_BY_PROPERTY), "Only support setting 'sample_by' property");
-
-        ImmutableList.Builder<String> tableOptions = ImmutableList.builder();
-        ClickHouseTableProperties.getSampleBy(properties).ifPresent(value -> tableOptions.add("SAMPLE BY " + value));
-
-        try (Connection connection = connectionFactory.openConnection(session)) {
-            String sql = format(
-                    "ALTER TABLE %s MODIFY %s",
-                    quoted(handle.asPlainTable().getRemoteTableName()),
-                    join(" ", tableOptions.build()));
-            execute(connection, sql);
-        }
-        catch (SQLException e) {
-            throw new TrinoException(JDBC_ERROR, e);
-        }
-    }
-
     @Override
     protected String getColumnDefinitionSql(ConnectorSession session, ColumnMetadata column, String columnName)
     {