Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disallow using reserved column names for Delta CDF tables #16953

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Disallow using reserved column names for Delta CDF tables #16953

merged 1 commit into from
Apr 12, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Apr 11, 2023

Description

Disallow using reserved column names for Delta CDF tables
Fixes #16913

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Disallow using reserved column names `_change_type`, `_commit_version` and `_commit_timestamp` 
  when creating a table or add a column with change data feed. ({issue}`16913`)
* Disallow enabling change data feed when the table contains 
  `_change_type`, `_commit_version` and `_commit_timestamp` columns. ({issue}`16913`)

@cla-bot cla-bot bot added the cla-signed label Apr 11, 2023
@ebyhr ebyhr self-assigned this Apr 11, 2023
@ebyhr ebyhr force-pushed the ebi/delta-cdf-column-names branch from 715e8b1 to 2d0cea4 Compare April 11, 2023 03:05
@github-actions github-actions bot added the delta-lake Delta Lake connector label Apr 11, 2023
@ebyhr ebyhr requested review from krvikash and findinpath April 11, 2023 08:06
@findinpath findinpath requested a review from homar April 11, 2023 09:09
@findinpath
Copy link
Contributor

This PR relates to #15821

@ebyhr ebyhr requested a review from findepi April 11, 2023 09:30
findinpath
findinpath reviewed Apr 11, 2023
View reviewed changes
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated
{
Set<String> conflicts = Sets.intersection(columnNames, CHANGE_DATA_FEED_COLUMN_NAMES);
if (!conflicts.isEmpty()) {
throw new TrinoException(NOT_SUPPORTED, "Unable to use %s when change data feed is enabled: %s".formatted(CHANGE_DATA_FEED_COLUMN_NAMES, conflicts));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unable to use %s -> Unable to use %s column name(s)

Unable to use [_change_type] when change data feed is enabled

Let's cosmetize a bit the message shown to the end user - maybe use a string joiner to add quotes to each illegal column name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we could also indicate that the column names are reserved.

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
krvikash
krvikash reviewed Apr 11, 2023
View reviewed changes
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated
{
Set<String> conflicts = Sets.intersection(columnNames, CHANGE_DATA_FEED_COLUMN_NAMES);
if (!conflicts.isEmpty()) {
throw new TrinoException(NOT_SUPPORTED, "Unable to use %s when change data feed is enabled: %s".formatted(CHANGE_DATA_FEED_COLUMN_NAMES, conflicts));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we could also indicate that the column names are reserved.

...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeMinioConnectorTest.java Show resolved Hide resolved
findepi
findepi approved these changes Apr 11, 2023
View reviewed changes
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
...rino-delta-lake/src/test/java/io/trino/plugin/deltalake/BaseDeltaLakeMinioConnectorTest.java
{
try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_column", "(col int) WITH (change_data_feed_enabled = true)")) {
assertQueryFails(
"ALTER TABLE " + table.getName() + " ADD COLUMN " + columnName + " int",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add also a test with RENAME COLUMN (the test table should use column mapping mode)
currently not supported, but will ensure we keep desired test coverage when implementing rename column feature

Copy link
Member Author

@ebyhr ebyhr Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#15821 already contains the test to deny CDF column names in RENAME COLUMN. Let me skip adding the test to this PR.

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/delta-cdf-column-names branch from 2d0cea4 to d39e875 Compare April 12, 2023 00:25
@ebyhr ebyhr force-pushed the ebi/delta-cdf-column-names branch from d39e875 to ba326e7 Compare April 12, 2023 02:41
@ebyhr ebyhr merged commit a8cf790 into trinodb:master Apr 12, 2023
@ebyhr ebyhr deleted the ebi/delta-cdf-column-names branch April 12, 2023 05:09
@github-actions github-actions bot added this to the 413 milestone Apr 12, 2023
@ebyhr ebyhr mentioned this pull request Apr 12, 2023
Closed
@colebow colebow mentioned this pull request Apr 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krvikash krvikash krvikash left review comments

@findinpath findinpath findinpath approved these changes

@findepi findepi findepi approved these changes

@homar homar homar approved these changes

Assignees

@ebyhr ebyhr

Labels
cla-signed delta-lake Delta Lake connector
Milestone
413
Development

Successfully merging this pull request may close these issues.

Disallow using reserved column names while creating a Delta CDF table
5 participants
@ebyhr @findinpath @homar @findepi @krvikash