Deny adding column with comment if the connector does not support this feature by findinpath · Pull Request #11493 · trinodb/trino

findinpath · 2022-03-15T11:25:14Z

Description

Deny adding column with comment if the connector does not support this feature

Is this change a fix, improvement, new feature, refactoring, or other?

Refactoring

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

All the connectors

How would you describe this change to a non-technical end user or system administrator?

Deny adding column with comment if the connector does not support this feature

Related issues, pull requests, and links

Fixes #11486

Documentation

(x) No documentation is needed.
( ) 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
* Deny adding column with comment if the connector does not support this feature. ({issue}`11486`)

# Kudu
* Add support for adding columns with comment. ({issue}`11486`)

.

findepi · 2022-03-15T13:31:24Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

BCT should provide sufficient coverage for adding column with comment. The test doesn't need to be Iceberg-specific.

cc @hashhar @ebyhr

Yes, I filed #11486 yesterday.

I moved the test to BaseConnectorTest.
If the column comment functionality is not available on ADD COLUMN we'll probably need a new entry in TestingConnectorBehavior enum

Yes, I filed #11486 yesterday.

thanks!

please make sure this Iceberg-specific test method gets removed as part of that issue

I've largely shrinked the test to concentrate only on adding a column with comment.
However, there is a series of connectors (e.g.: Postgres, MsSQL) which do support adding columns, but ignore the comment provided.

I'll add a new testing behaviour SUPPORTS_ADD_COLUMN_WITH_COMMENT .

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

findepi · 2022-03-16T09:11:05Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

it would be weird for a connector to support ADD COLUMN without CREATE TABLE.
I think we can drop this line.

in the worst case, a connector that has exactly such weird behavior will have to override this method, or we bring the condition back (which will perhaps never happen)

yeah skipping would just hide such "broken" (IMO) connectors.

Elasticsearch, Redis seems not to support creating tables, but can add columns

why do you think Elasticsearch, Redis support adding columns?

Sorry. My mistake. I misread the test results.

I think we can drop this line.

let's drop the line

Dropped the line and also adjusted Redis & Elasticsearch tests to provide appropriate values for the SUPPORTS_ADD_COLUMN behavior. Thank you (yet again) @findepi for stressing on this aspect.

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

ebyhr · 2022-03-16T10:45:56Z

Fixes #11162
Fixes #11486

The added test verifies table comments only when the connector supports ADD COLUMN statement. I think we shouldn't close #11162 unless we fix BaseConnectorTest.testCommentColumn() or add another test in this PR.

hashhar

LGTM from me

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

findepi · 2022-03-21T09:53:30Z

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

This should be handled by declaring that it doesn't have SUPPORTS_ADD_COLUMN_WITH_COMMENT behavior, and checked that the connector indeed rejects column with a comment.

Actually the clickhouse connector test supports adding columns only for MergeTree engine tables:

trino/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java

Lines 144 to 145 in 70ad1f9

// Only MergeTree engine table can add column

assertUpdate("CREATE TABLE " + tableName + " (id int NOT NULL, x VARCHAR) WITH (engine = 'MergeTree', order_by = ARRAY['id'])");

However, the column comment is ignored at the moment. This is the reason why the implementation of the test for the Clickhouse connector simply skips testing the functionality.

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

findepi · 2022-03-21T09:55:42Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

else (when ! hasBehavior(SUPPORTS_ADD_COLUMN_WITH_COMMENT)) the ALTER TABLE should have failed

I don't fully understand what you mean.

If the connector does support adding columns without comment it will simply ignore the comment provided, but the ADD COLUMN statement will nevertheless succeed.

If the connector does support adding columns without comment it will simply ignore the comment provided

This is a bug, see #11348 (talks about table comments, but same story)

I have updated the PR to reflect this new aspect.
In case that the connector does not support adding a column with comment, it will fail to execute the ADD COLUMN task when the comment is present.

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java

findepi · 2022-03-22T14:57:26Z

fyi CI red

…nctionality

findepi · 2022-04-01T08:02:46Z

@hashhar @ebyhr @phd3 PTAL

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

ebyhr

Please fix a test failure in SignleStore connector.

…s feature Instead of silently ignoring the column comment if the connector does not support adding column comments, throw explicitly an exception to expose the fact that this feature is not supported.

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

findinpath requested review from ebyhr and hashhar March 15, 2022 11:25

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 49bd2ba to 3df752d Compare March 15, 2022 11:26

findepi previously approved these changes Mar 15, 2022

View reviewed changes

findepi reviewed Mar 15, 2022

View reviewed changes

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 3df752d to 383a386 Compare March 16, 2022 05:59

findepi changed the title ~~Rename method to better suite the purpose of the test~~ Move test for adding column with comment to BaseConnectorTest Mar 16, 2022

findepi reviewed Mar 16, 2022

View reviewed changes

hashhar reviewed Mar 16, 2022

View reviewed changes

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch 3 times, most recently from 08ccda6 to 7638c46 Compare March 16, 2022 15:29

hashhar approved these changes Mar 16, 2022

View reviewed changes

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java Outdated Show resolved Hide resolved

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java Outdated Show resolved Hide resolved

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 7638c46 to 1492897 Compare March 18, 2022 12:28

findepi reviewed Mar 21, 2022

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved

findepi reviewed Mar 21, 2022

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java Outdated Show resolved Hide resolved

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch 3 times, most recently from 14f164b to 563b956 Compare March 22, 2022 08:02

findinpath changed the title ~~Move test for adding column with comment to BaseConnectorTest~~ Deny adding column with comment if the connector does not support this feature Mar 22, 2022

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch 2 times, most recently from 0c5a071 to 7e33573 Compare March 22, 2022 10:21

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch 2 times, most recently from 076c28b to 7a989ea Compare March 23, 2022 11:19

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 7a989ea to 9e74970 Compare March 24, 2022 15:38

Disable support for adding columns in Druid

a78d4ae

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 6b57929 to 357338b Compare March 25, 2022 05:03

Adjust SUPPORTS_ADD_COLUMN setting for the connectors missing this fu…

94897f5

…nctionality

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch 2 times, most recently from 5c6b410 to 94f7172 Compare March 26, 2022 20:37

Add support for adding columns with comments in Kudu

9174448

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 94f7172 to 2da33a5 Compare March 27, 2022 08:23

findinpath requested review from findepi and hashhar March 27, 2022 10:11

findepi removed their request for review April 1, 2022 08:02

ebyhr reviewed Apr 1, 2022

View reviewed changes

plugin/trino-mongodb/src/test/java/io/trino/plugin/mongodb/BaseMongoConnectorTest.java Outdated Show resolved Hide resolved

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated Show resolved Hide resolved

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from 2da33a5 to 8994f03 Compare April 1, 2022 14:29

findinpath requested a review from ebyhr April 1, 2022 14:29

ebyhr mentioned this pull request Apr 4, 2022

CI: Deny adding column with comment if the connector does not support this feature #11773

Closed

ebyhr requested changes Apr 4, 2022

View reviewed changes

Deny adding column with comment if the connector does not support thi…

96de89d

…s feature Instead of silently ignoring the column comment if the connector does not support adding column comments, throw explicitly an exception to expose the fact that this feature is not supported.

findinpath force-pushed the iceberg-test-add-column-with-comment-rename branch from be3baab to 96de89d Compare April 4, 2022 12:37

ebyhr approved these changes Apr 4, 2022

View reviewed changes

ebyhr merged commit ec3aa36 into trinodb:master Apr 4, 2022

github-actions bot added this to the 376 milestone Apr 4, 2022

ebyhr mentioned this pull request Apr 4, 2022

Release notes for 376 #11692

Closed

mosabua mentioned this pull request Apr 5, 2022

Add Trino 376 release notes #11691

Merged

ebyhr mentioned this pull request Sep 2, 2022

Deny comment on unsupported table #11430

Closed

	// Only MergeTree engine table can add column
	assertUpdate("CREATE TABLE " + tableName + " (id int NOT NULL, x VARCHAR) WITH (engine = 'MergeTree', order_by = ARRAY['id'])");

Conversation

findinpath commented Mar 15, 2022 • edited by ebyhr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath Mar 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ebyhr commented Mar 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi commented Mar 22, 2022

Uh oh!

findepi commented Apr 1, 2022

Uh oh!

Uh oh!

Uh oh!

ebyhr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

findinpath commented Mar 15, 2022 •

edited by ebyhr

Loading

findinpath Mar 18, 2022 •

edited

Loading

ebyhr commented Mar 16, 2022 •

edited

Loading

ebyhr left a comment •

edited

Loading