Skip to content

Add redirection awareness for RENAME table operation#11277

Merged
findepi merged 2 commits intotrinodb:masterfrom
findinpath:rename-table-redirection-awareness
Mar 30, 2022
Merged

Add redirection awareness for RENAME table operation#11277
findepi merged 2 commits intotrinodb:masterfrom
findinpath:rename-table-redirection-awareness

Conversation

@findinpath
Copy link
Contributor

Description

Add redirection awareness for the ALTER TABLE xxx RENAME TO yyy statement.

The implementation follows the specification from #11202 (comment)

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

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

This implementation affects the core query engine.

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

In the context of working in an environment where Hive/Iceberg/Delta tables are mixed, allow the user to do a table rename by deriving the destination catalog and schema from the source table name.

When having the table redirection enabled in the hive connector (via hive.iceberg-catalog-name=iceberg in hive.properties) and a table iceberg.default.iceberg_table_name within the iceberg connector, any of the following statements is considered valid:

     ALTER TABLE hive.default.iceberg_table_name to iceberg_table_name_new;
     ALTER TABLE hive.default.iceberg_table_name to default.iceberg_table_name_new;
     ALTER TABLE hive.default.iceberg_table_name to iceberg.default.iceberg_table_name_new;

Related issues, pull requests, and links

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:

# Main
* Add redirection awareness for RENAME table operation

Copy link
Member

Choose a reason for hiding this comment

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

define a newTableName variable

Copy link
Member

Choose a reason for hiding this comment

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

why target -> source ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a mistake. Thank you for raising this point.

Copy link
Member

Choose a reason for hiding this comment

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

do we have test coverage for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a few tests:

  • io.trino.security.TestAccessControl#testNonQueryAccessControl -> is using a rather naive io.trino.testing.TestingAccessControlManager behind the scenes (and no systemAccessControls)
  • io.trino.testing.BaseConnectorTest#testRenameTable

However I haven't found a test interacting with a fine grained system access control implementation like https://trino.io/docs/current/security/file-system-access-control.html

Copy link
Member

Choose a reason for hiding this comment

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

Did any test fail when you had target here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. There were no failures as far as I can recall.
https://github.com/trinodb/trino/actions/runs/1922889046

Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a test? Orthogonal to this PR

Copy link
Member

Choose a reason for hiding this comment

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

actually…. there should be two tests: we should have an access control test for rename; and we should have an access control test for rename with redirects; we should verify which table AC is asked about

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #11488 as a scaffolding for dealing with access control tests for redirections. I'll be adding support for RENAME TABLE there once it reaches master.

@findinpath findinpath force-pushed the rename-table-redirection-awareness branch 2 times, most recently from 091a4a2 to 1b9e215 Compare March 2, 2022 14:53
@findinpath findinpath force-pushed the rename-table-redirection-awareness branch from 1b9e215 to 77e4d1c Compare March 23, 2022 15:54
@findinpath findinpath requested a review from findepi March 23, 2022 15:57
@findinpath findinpath force-pushed the rename-table-redirection-awareness branch from 77e4d1c to 049e645 Compare March 25, 2022 14:23
@findinpath findinpath force-pushed the rename-table-redirection-awareness branch from 049e645 to 29375ae Compare March 28, 2022 10:31
@findepi findepi merged commit 81273fe into trinodb:master Mar 30, 2022
@github-actions github-actions bot added this to the 376 milestone Mar 30, 2022
@findepi findepi mentioned this pull request Mar 31, 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.

2 participants