-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 FIRST
, AFTER
and LAST
clause when adding a new column in engine and Iceberg connector
#20914
Conversation
4cd3d46
to
a111e6d
Compare
From a Zoom Meeting with @martint , @electrum
|
a111e6d
to
1c8d5d8
Compare
1c8d5d8
to
4966026
Compare
4966026
to
6308a34
Compare
6308a34
to
b3f2425
Compare
b3f2425
to
f2ab14b
Compare
952d1a0
to
3685a36
Compare
2114dec
to
8b60661
Compare
8b60661
to
7bc3e73
Compare
f9cb8b0
to
21fe7b3
Compare
21fe7b3
to
1476046
Compare
If we add FIRST .. why not also add LAST? |
Also .. is there any chance we implement this later for JDBC connectors and others across the board? |
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.
The single-level scenario syntax looks good.
The nested case is somewhat confusing. At first glance, it's not clear what y
refers to in this statement:
ALTER TABLE t ADD COLUMN a.b.c BIGINT AFTER y
It's meant to be a field within a.b
, but that's not immediately obvious. It could, as well, be a top-level column and the statement could be a short hand for creating all the required nested fields with the root a
after y
.
The following structure would be more natural, but it only works with AFTER
and it's incompatible with how nested columns are handled in general:
ALTER TABLE t ADD COLUMN c BIGINT AFTER a.b.y
All this is to say that I need to think more about the nested case.
core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
I guess that already exists. |
Yes, we can support this option in JDBC and other connectors if the datasource supports specifying columns positions. |
1476046
to
14dc059
Compare
Addressed comments. |
14dc059
to
3b94481
Compare
FIRST
and AFTER
clause when adding a new column in engine and Iceberg connectorFIRST
, AFTER
and LAST
clause when adding a new column in engine and Iceberg connector
FYI, putting fully qualified identifier after |
We decided to exclude nested fields' support from this PR. I will file an issue and update this PR today or tomorrow. |
3b94481
to
2a810fc
Compare
(Rebased on master without any changes) |
2a810fc
to
de97c9f
Compare
core/trino-parser/src/main/java/io/trino/sql/tree/ColumnPosition.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/connector/ColumnPosition.java
Outdated
Show resolved
Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/AddColumn.java
Outdated
Show resolved
Hide resolved
de97c9f
to
0c5c12c
Compare
Description
I propose adding
FIRST
,AFTER
andLAST
options toALTER TABLE ... ADD COLUMN
statement so that we can add new columns and fields to specific positions.LAST
is the default when the option isn't provided.Column case:
Datasources supporting these options:
Fixes #20091
Release notes
(x) Release notes are required, with the following suggested text: