Support ALTER COLUMN SET DATA TYPE statement #11608

ebyhr · 2022-03-22T05:51:44Z

Description

Support ALTER COLUMN SET DATA TYPE statement. ANSI specification:

<alter table statement> ::=
  ALTER TABLE <table name>  <alter table action> 

<alter table action> ::=
...
  | <alter column definition> 

<alter column definition> ::=
ALTER [ COLUMN ] <column name> <alter column action>

<alter column action> ::=
...
  | <alter column data type clause> 

<alter column data type clause> ::=
  SET DATA TYPE <data type>

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# General
* Add support for `ALTER COLUMN ... SET DATA TYPE` statement. ({issue}`11608 `)

Fixes #10736

ebyhr · 2022-03-24T01:07:20Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

+     *
+     * @throws AccessDeniedException if not allowed
+     */
+    void checkCanAlterColumn(SecurityContext context, QualifiedObjectName tableName);


@martint @kokosing Do you think if we should check the permission in ALTER COLUMN level? Or it should be SET DATA TYPE (= checkCanSetColumnType) level?

I wonder if checkCanAlterTable is not enough here. However this can be modeled by the implementation of the access control easily. So I think checkCanAlterColumn is good.

According to the SQL spec:

The enabled authorization identifiers shall include the <authorization identifier> that owns the schema identified by the <schema name> of the table identified by <table name>.

bitsondatadev · 2022-11-19T07:08:26Z

👋 @ebyhr - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

findepi · 2022-12-08T10:24:11Z

@ebyhr can you please chop it a little bit?

let's have a grammar commit (antlr grammar, astbuilder, sqlformatter)
let's have a statement commit (let's throw all the rest there for now)

let's also move mongo stuff to separate PR; i tend not to review Mongo-related stuff

ebyhr · 2022-12-09T03:13:49Z

Separate into two commits and removed a MongoDB commit.

core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java

ebyhr · 2022-12-12T04:31:59Z

CI hit #15367

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4

alexjo2144 · 2022-12-12T15:56:29Z

core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java

+        accessControl.checkCanAlterColumn(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(qualifiedObjectName));
+
+        TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
+        Map<String, ColumnHandle> columnHandles = metadata.getColumnHandles(session, tableHandle);


Should you make sure the user can see the column here using accessControl.filterColumns?

I'm not sure, other column tasks (e.g. DROP COLUMN) don't have such logic.

@kokosing Do you have any opinion?

It seems that the accessControl.filterColumns check is relevant currently only within information_schema.columns metadata table.

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

core/trino-parser/src/main/java/io/trino/sql/tree/SetColumnType.java

findinpath · 2022-12-15T05:39:09Z

core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java

+        accessControl.checkCanAlterColumn(session.toSecurityContext(), redirectionAwareTableHandle.getRedirectedTableName().orElse(qualifiedObjectName));
+
+        TableHandle tableHandle = redirectionAwareTableHandle.getTableHandle().get();
+        Map<String, ColumnHandle> columnHandles = metadata.getColumnHandles(session, tableHandle);


It seems that the accessControl.filterColumns check is relevant currently only within information_schema.columns metadata table.

findinpath · 2022-12-15T05:49:56Z

core/trino-main/src/main/java/io/trino/testing/TestingMetadata.java

+        List<ColumnMetadata> columns = new ArrayList<>(tableMetadata.getColumns());
+        ColumnMetadata columnMetadata = getColumnMetadata(session, tableHandle, column);
+        columns.set(columns.indexOf(columnMetadata), ColumnMetadata.builderFrom(columnMetadata).setType(type).build());
+        tables.put(tableName, new ConnectorTableMetadata(tableName, ImmutableList.copyOf(columns), tableMetadata.getProperties(), tableMetadata.getComment()));


nit: Is the copy of the columns actually necessary?

findinpath · 2022-12-15T05:58:44Z

core/trino-parser/src/main/java/io/trino/sql/tree/SetColumnType.java

+    }
+
+    @Override
+    public boolean shallowEquals(Node other)


Is this override needed?

hopefully not needed, please remove

(the current impl isn't correct yet, it should check all local fields)

The javadoc of shallowEquals method says below. I don't think we should check all fields. I will remove either though.

Compare with another node by considering internal state excluding any Node returned by getChildren()

It seems you might be right. Still -- let's try to remove it, and avoid need for discussing it.
For example, AddColumn doesn't implement this

It seems you might be right. Still -- let's try to remove it, and avoid need for discussing it.

@ebyhr thoughts?

findepi · 2022-12-19T09:37:37Z

core/trino-parser/src/main/java/io/trino/sql/tree/SetColumnType.java

-    }
-
-    private SetColumnType(Optional<NodeLocation> location, QualifiedName tableName, Identifier columnName, DataType type, boolean tableExists)
+    public SetColumnType(Optional<NodeLocation> location, QualifiedName tableName, Identifier columnName, DataType type, boolean tableExists)


location is always known (except for io.trino.execution.TestSetColumnTypeTask#executeSetColumnType where you can fill it with dummy value), so no need for optional?

findepi · 2022-12-19T09:39:48Z

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

+    @Test
+    public void testAlterColumnSetDataType()
+    {
+        assertStatement("ALTER TABLE foo.t ALTER COLUMN a SET DATA TYPE bigint",


assertStatement is deprecated (although my IDE doesn't highlight that, perhaps because this is same class)

use assertThat(statement(....)).

findepi · 2022-12-19T09:43:28Z

core/trino-parser/src/main/java/io/trino/sql/tree/SetColumnType.java

+    }
+
+    @Override
+    public boolean shallowEquals(Node other)


hopefully not needed, please remove

(the current impl isn't correct yet, it should check all local fields)

ebyhr · 2022-12-19T10:52:11Z

Addressed comments.

findepi · 2022-12-21T13:09:51Z

(squashed and rebased)

findepi · 2022-12-21T13:11:14Z

core/trino-parser/src/main/java/io/trino/sql/tree/SetColumnType.java

    public SetColumnType(NodeLocation location, QualifiedName tableName, Identifier columnName, DataType type, boolean tableExists)
    {
-        this(Optional.of(location), tableName, columnName, type, tableExists);


changes in this class should all go in the first commit

findepi

% commit boundaries alignment and shallowEquals removal

Incorporate following changes Pass properties when checking access for CREATE SCHEMA trinodb/trino#15153 trinodb/trino@e16620f Support ALTER COLUMN SET DATA TYPE statement trinodb/trino#11608 trinodb/trino@ef94c0d

ebyhr added the enhancement New feature or request label Mar 22, 2022

cla-bot bot added the cla-signed label Mar 22, 2022

github-actions bot added docs tests:hive labels Mar 22, 2022

ebyhr force-pushed the ebi/alter-table-set-data-type branch 3 times, most recently from 04a6a48 to 9344ed4 Compare March 22, 2022 09:29

findepi added the syntax-needs-review label Mar 23, 2022

ebyhr commented Mar 24, 2022

View reviewed changes

ebyhr marked this pull request as ready for review March 28, 2022 01:48

ebyhr requested a review from kasiafi April 23, 2022 00:35

ebyhr force-pushed the ebi/alter-table-set-data-type branch 3 times, most recently from 0d98dcc to 188323d Compare May 17, 2022 01:40

ebyhr force-pushed the ebi/alter-table-set-data-type branch 3 times, most recently from 52ec290 to f92141a Compare July 7, 2022 05:29

ebyhr requested a review from martint July 8, 2022 03:18

ebyhr force-pushed the ebi/alter-table-set-data-type branch from f92141a to bcd411b Compare July 12, 2022 03:03

ebyhr force-pushed the ebi/alter-table-set-data-type branch from bcd411b to e4ce14c Compare July 26, 2022 05:07

ebyhr force-pushed the ebi/alter-table-set-data-type branch from e4ce14c to 6699757 Compare August 10, 2022 02:07

ebyhr force-pushed the ebi/alter-table-set-data-type branch from 6699757 to d32f968 Compare August 18, 2022 11:02

ebyhr requested a review from electrum August 18, 2022 11:02

ebyhr force-pushed the ebi/alter-table-set-data-type branch from d32f968 to 2c321a2 Compare September 13, 2022 07:17

ebyhr force-pushed the ebi/alter-table-set-data-type branch 2 times, most recently from 6d7ca24 to b01be62 Compare October 6, 2022 07:40

ebyhr force-pushed the ebi/alter-table-set-data-type branch from b01be62 to 14a445d Compare October 14, 2022 07:27

ebyhr force-pushed the ebi/alter-table-set-data-type branch 2 times, most recently from 6f137a4 to d23f22c Compare November 1, 2022 05:43

findepi mentioned this pull request Dec 8, 2022

Add ALTER TABLE syntax to handle column type change #10736

Closed

findepi reviewed Dec 8, 2022

View reviewed changes

core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved

ebyhr force-pushed the ebi/alter-table-set-data-type branch from a305366 to 8df8da2 Compare December 9, 2022 03:13

findinpath reviewed Dec 9, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java Outdated Show resolved Hide resolved

ebyhr force-pushed the ebi/alter-table-set-data-type branch from 8df8da2 to 34d7298 Compare December 12, 2022 02:51

alexjo2144 reviewed Dec 12, 2022

View reviewed changes

findepi reviewed Dec 14, 2022

View reviewed changes

findinpath reviewed Dec 15, 2022

View reviewed changes

ebyhr requested a review from findepi December 16, 2022 04:47

findepi reviewed Dec 19, 2022

View reviewed changes

martint force-pushed the master branch from 40dbb4f to 0d73d10 Compare December 19, 2022 20:15

findepi force-pushed the ebi/alter-table-set-data-type branch from 30e57f9 to e0a1f6e Compare December 21, 2022 13:09

findepi reviewed Dec 21, 2022

View reviewed changes

findepi approved these changes Dec 21, 2022

View reviewed changes

Support ALTER COLUMN SET DATA TYPE in grammar

36444a6

ebyhr force-pushed the ebi/alter-table-set-data-type branch from e0a1f6e to 1db772f Compare December 21, 2022 13:23

Support ALTER COLUMN SET DATA TYPE statement

b85a786

ebyhr force-pushed the ebi/alter-table-set-data-type branch from 1db772f to b85a786 Compare December 21, 2022 23:49

ebyhr merged commit ef94c0d into trinodb:master Dec 23, 2022

ebyhr deleted the ebi/alter-table-set-data-type branch December 23, 2022 00:55

ebyhr mentioned this pull request Dec 23, 2022

Release notes for 405 #15058

Closed

github-actions bot added this to the 404 milestone Dec 23, 2022

colebow mentioned this pull request Dec 28, 2022

Add Trino 405 release notes #15139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ALTER COLUMN SET DATA TYPE statement #11608

Support ALTER COLUMN SET DATA TYPE statement #11608

ebyhr commented Mar 22, 2022 •

edited

Loading

ebyhr Mar 24, 2022

kokosing Mar 24, 2022

martint Jul 6, 2022

bitsondatadev commented Nov 19, 2022

findepi commented Dec 8, 2022

ebyhr commented Dec 9, 2022

ebyhr commented Dec 12, 2022

alexjo2144 Dec 12, 2022

ebyhr Dec 14, 2022

findinpath Dec 15, 2022

findinpath Dec 15, 2022

findinpath Dec 15, 2022

findinpath Dec 15, 2022

findepi Dec 19, 2022

ebyhr Dec 19, 2022 •

edited

Loading

findepi Dec 19, 2022

findepi Dec 21, 2022

findepi Dec 19, 2022

findepi Dec 19, 2022

findepi Dec 19, 2022

ebyhr commented Dec 19, 2022

findepi commented Dec 21, 2022

findepi Dec 21, 2022

findepi left a comment

Support ALTER COLUMN SET DATA TYPE statement #11608

Support ALTER COLUMN SET DATA TYPE statement #11608

Conversation

ebyhr commented Mar 22, 2022 • edited Loading

Description

Documentation

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitsondatadev commented Nov 19, 2022

findepi commented Dec 8, 2022

ebyhr commented Dec 9, 2022

ebyhr commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Dec 19, 2022

findepi commented Dec 21, 2022

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

ebyhr commented Mar 22, 2022 •

edited

Loading

ebyhr Dec 19, 2022 •

edited

Loading