-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support adding a field with ADD COLUMN in Iceberg #16321
Conversation
2c066dd
to
e91f18d
Compare
e7ac1ef
to
50e5080
Compare
59eb1b7
to
c39c2e8
Compare
c39c2e8
to
046d27b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Parse column name as QualifiedName in column definition"
core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java
Outdated
Show resolved
Hide resolved
046d27b
to
6214fc5
Compare
Addressed comments. |
core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java
Outdated
Show resolved
Hide resolved
core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4
Outdated
Show resolved
Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/ColumnDefinition.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebyhr @findepi in my opinion this new functionality is not "ADD COLUMN". It effectively doesn't add a column, but it changes the type of an existing column. Hence, it falls under "ALTER COLUMN".
The user could probably use the existing syntax ALTER COLUMN ... SET DATA TYPE ...
to add a nested field. I understand however that this syntax is very verbose, as it requires to explicitly describe the requested type. I think that we could extend the ALTER COLUMN
syntax to support adding a nested field in a more concise form.
It could be something like:
ALTER COLUMN <column_name> ADD FIELD <field_name> <type>
The <field_name> could be an identifier for an immediately nested field, and a qualified name for deeper nesting (anyway, the passed name should be relative to the <column_name>).
Execution-wise, the two ways of adding a column, ALTER COLUMN ... SET DATA TYPE ...
and ALTER COLUMN ... ADD FIELD ...
should be unified. The new syntax should be considered syntactic sugar.
All the above applies as well to removing nested fields (#16002).
Yes, adding nested fields should be handled with SET DATA TYPE. We talked about it nested fields when adding support for that functionality, but stopped before defining the syntax required. |
I agree that adding nested fields should be possible via SET DATA TYPE. it is, however, impractical to require users to provide current ROW definition plus the new field when they want to add a new field, that's why we need option to add/remove individual fields, or users will not use Trino for that. I don't think there is any disagreement, I just wanted to make sure others have the context.
I would be hesitant about that. |
@martint Could you take another look? |
daddbcd
to
47869e0
Compare
Rebased on master to resolve conflicts. |
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
47869e0
to
01823e9
Compare
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
41f66f8
to
53d0cf8
Compare
Rebased on master to resolve conflicts. |
53d0cf8
to
fa5e8ae
Compare
@martint Could you please take another look? |
@ebyhr the build isn't green |
The failure #18202 is unrelated to this change. |
@Override | ||
public String getName() | ||
{ | ||
return "ADD COLUMN"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call it add field
core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
fa5e8ae
to
c6362de
Compare
c6362de
to
c2a2252
Compare
Let me merge this PR today if there are no additional comments. |
I’m still reviewing |
Description
Relates to #16897
Fixes #16248
Release notes
(x) Release notes are required, with the following suggested text: