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.

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

Document support for ALTER TABLE SET PROPERTIES #11022

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

jhlodin
Copy link
Contributor

@jhlodin jhlodin commented Feb 11, 2022

Description

ALTER TABLE SET PROPERTIES has been supported for a while, but has been missing practical details such as detailed examples, use of the DEFAULT keyword, and what connectors (don't) support it.

General information

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

Improvement

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

Documentation

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

Further document support and use of ALTER TABLE SET PROPERTIES

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) 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

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

# Section
* Fix some things. ({issue}`5678`)

@cla-bot cla-bot bot added the cla-signed label Feb 11, 2022
@jhlodin jhlodin requested a review from hashhar February 11, 2022 15:41
@jhlodin jhlodin added the docs label Feb 11, 2022
@jhlodin jhlodin requested review from Ordinant and rosewms February 11, 2022 15:43
@@ -82,9 +98,19 @@ Allow everyone with role public to drop and alter table ``people``::

ALTER TABLE people SET AUTHORIZATION ROLE PUBLIC
Copy link
Member

@Ordinant Ordinant Feb 11, 2022

Choose a reason for hiding this comment

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

The interaction between pure SQL role granting and access control role granting is ... unexplored.

Copy link
Contributor Author

@jhlodin jhlodin Feb 11, 2022

Choose a reason for hiding this comment

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

Understood, but ultimately out of scope for this PR. If someone has input then I can file a follow-up issue

Copy link
Member

Choose a reason for hiding this comment

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

@Ordinant Do you mean using things like file-based access control together with SQL based access control?

If so such a configuration can never be possible since Trino only allows a single type of system access control to be available at a time.

However connectors may provide their own - and the rule there is that system level configuration wins over connector specific ones.

Copy link
Member

@Ordinant Ordinant left a comment

Choose a reason for hiding this comment

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

Approved, with quibbles. The ticket probably should be titled "Document lack of support for ..."

@jhlodin jhlodin force-pushed the jl/alter-table-set-properties branch from 8ad8349 to f443264 Compare February 11, 2022 16:37
@hashhar hashhar requested a review from joshthoward February 11, 2022 17:02
@hashhar
Copy link
Member

hashhar commented Feb 11, 2022

cc: @joshthoward for technical correctness

Copy link
Contributor

@rosewms rosewms left a comment

Choose a reason for hiding this comment

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

LGTM pending comments

@jhlodin jhlodin force-pushed the jl/alter-table-set-properties branch 2 times, most recently from 1e4bdf2 to aaa91bd Compare March 2, 2022 19:09
@jhlodin jhlodin requested a review from ebyhr March 3, 2022 18:18
@jhlodin jhlodin force-pushed the jl/alter-table-set-properties branch 2 times, most recently from 756fbca to f29e556 Compare March 4, 2022 18:51
@jhlodin
Copy link
Contributor Author

jhlodin commented Mar 4, 2022

Discussed with @electrum and agreed that it's probably too granular to devote a sub-section to connector pages that says ALTER TABLE ... SET PROPERTIES is unsupported, especially because the error message is explicit enough about not supporting changing table properties. Removed the "unsupported" fragment and just leaving the core documentation of the clause.

@jhlodin jhlodin force-pushed the jl/alter-table-set-properties branch from f29e556 to ef7583f Compare March 4, 2022 18:55
@jhlodin jhlodin force-pushed the jl/alter-table-set-properties branch from ef7583f to 2cce907 Compare March 4, 2022 18:56
@jhlodin
Copy link
Contributor Author

jhlodin commented Mar 7, 2022

@electrum Can we merge this?

@electrum electrum merged commit 7a96ffc into trinodb:master Mar 7, 2022
@jhlodin jhlodin deleted the jl/alter-table-set-properties branch March 7, 2022 23:27
@github-actions github-actions bot added this to the 373 milestone Mar 7, 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.

Document ALTER TABLE SET PROPERTIES
5 participants