Skip to content

Add test to verify failure for renaming to long table name#13289

Merged
ebyhr merged 5 commits intotrinodb:masterfrom
ebyhr:ebi/test-rename-to-long-identifier
Aug 9, 2022
Merged

Add test to verify failure for renaming to long table name#13289
ebyhr merged 5 commits intotrinodb:masterfrom
ebyhr:ebi/test-rename-to-long-identifier

Conversation

@ebyhr
Copy link
Member

@ebyhr ebyhr commented Jul 21, 2022

Description

Add test to verify failure for renaming to long table name. The changes for MongoDB and SQL Server connectors are required to pass the test.
Fixes #13073

Documentation

(x) No documentation is needed.

Release notes

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

# MongoDB, SQL Server, PostgreSQL
* Prevent renaming to a table with a name longer than the max length.
  Previously, the name was truncated to the max length. ({issue}`13073`)

@cla-bot cla-bot bot added the cla-signed label Jul 21, 2022
@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch from 0a1f223 to dec06a7 Compare July 21, 2022 12:20
@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch 3 times, most recently from fc6eaea to e7f9d5b Compare July 23, 2022 13:45
@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch 2 times, most recently from b897557 to 9a07904 Compare July 29, 2022 23:53
@ebyhr ebyhr requested review from findepi and hashhar August 3, 2022 03:01
@ebyhr
Copy link
Member Author

ebyhr commented Aug 3, 2022

CI plugin/trino-redis failed by

Status 500: {"message":"Head \"[https://registry-1.docker.io/v2/testcontainers/ryuk/manifests/0.3.3\](https://registry-1.docker.io/v2/testcontainers/ryuk/manifests/0.3.3/)": Get \"https://auth.docker.io/token?account=githubactions&scope=repository%3Atestcontainers%2Fryuk%3Apull&service=registry.docker.io\": EOF"}

Copy link
Member

Choose a reason for hiding this comment

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

Long commit message - Ensure failure when creating invalid table name in testCreateTableWithLongTableName.

Would Verify failure for invalid table name in testCreateTableWithLongTableName work?

Copy link
Member

Choose a reason for hiding this comment

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

Nice, also seems to fix the issue that we would throw raw SQLException instead of wrapping into a TrinoException.

Copy link
Member

Choose a reason for hiding this comment

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

nit: Check target table name when renaming a table in MongoDB -> Check target table name length when renaming a table in MongoDB

Copy link
Member

Choose a reason for hiding this comment

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

The MongoDB docs talk about bytes and it seems they don't disallow multibyte characters so instead of checking string length we should check bytes probably.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Let me fix.

Copy link
Member

Choose a reason for hiding this comment

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

nit: the var should probably be renamed since it's bytes now instead of length.

Copy link
Member

Choose a reason for hiding this comment

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

nit: The comment is slightly confusing because it talks about 128 but does comparison with databaseMetadata.getMaxTableNameLength().

@findepi findepi removed their request for review August 3, 2022 08:57
@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch 4 times, most recently from 8283fd5 to 4db1f01 Compare August 8, 2022 02:08
@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch from 4db1f01 to cf6752d Compare August 8, 2022 09:30
Copy link
Member

Choose a reason for hiding this comment

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

nit: the var should probably be renamed since it's bytes now instead of length.

// The connector appends uuids to the end of all table names
return OptionalInt.of(255 - UUID.randomUUID().toString().length());
// 33 is the length of random suffix. e.g. {table name}-142763c594d54e4b9329a98f90528caf
return OptionalInt.of(255 - 33);
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

@ebyhr ebyhr Aug 8, 2022

Choose a reason for hiding this comment

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

Previously, this test passed in Iceberg unintentionally because of its try-catch style. This change is required to adjust to the right max length.

@ebyhr ebyhr force-pushed the ebi/test-rename-to-long-identifier branch from cf6752d to 6665e10 Compare August 8, 2022 23:19
@ebyhr
Copy link
Member Author

ebyhr commented Aug 9, 2022

CI hit #13107

@ebyhr ebyhr merged commit acdecbe into trinodb:master Aug 9, 2022
@ebyhr ebyhr deleted the ebi/test-rename-to-long-identifier branch August 9, 2022 10:38
@ebyhr ebyhr mentioned this pull request Aug 9, 2022
@github-actions github-actions bot added this to the 393 milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Add test for ALTER TABLE ... RENAME TO long identifier

2 participants