Add ALTER TABLE SET PROPERTIES statement

[pratyakshsharma]

Cherry-pick of trinodb/trino#9401

Co-authored-by: ebyhr

Motivation and Context

Setting table properties can help in modifying critical table properties such as the one which controls concurrency behaviour for an iceberg table.

Impact

ALTER TABLE ... SET PROPERTIES

Test Plan

Manual testing
Unit testing

Contributor checklist

• Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Add :doc:`/sql/alter-table` SET PROPERTIES statement :pr:`21495`

[github-actions]

Codenotify: Notifying subscribers in CODENOTIFY files for diff ba2f731...4f826af.

Notify	File(s)
@aditi-pandit	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@elharo	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@kaikalur	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@rschlussel	presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

[Akanksha-kedia]

single commit per PR and Can you update with [WIP] or draft if few changes are yet to be done.

I wanted to understand how existing properties are going to handled, can we add random values to existing properties? and also if we have alter table set properties, why not having alter schema/database and also as its taking random key = value wat value its adding and use case to it.

[steveburnett]

LGTM! (docs)

[pratyakshsharma]

@Akanksha-kedia

There are 2 separate cherry-picks which serve different purposes, hence 2 different commits.
I think verifications related to existing properties and what key value pairs are going to be set can be handled at connector level. Different table formats can have different key for possible table properties.
I am adding this syntax support because ALTER TABLE SET PROPERTIES is currently needed for iceberg to solve concurrency related issues. You can refer this for more details - #21251

[pratyakshsharma]

This is ready for review now @yingsu00 @imjalpreet @agrawalreetika

[pratyakshsharma]

After the recent push, build is failing. Working on this.

[pratyakshsharma]

This is ready to be reviewed.

[agrawalreetika]

I see the boolean parameter setDefaultProperties missed here, which was introduced in trino commit?

[pratyakshsharma]

I did not understand the use case of introducing that parameter, and hence skipped it.

[agrawalreetika]

Add Override annotation here.
Can we add some tests to test the feature support in the connector?

[pratyakshsharma]

Clickhouse server is spawned using docker in presto tests. I skipped the test on purpose as I am not sure how do I test it. cc @yingsu00

[agrawalreetika]

@pratyakshsharma Should we include ALTER TABLE SET PROPERTIES support commit in this PR itself? WDYT

[pratyakshsharma]

The test failures are related to the code changes from #21250. Once it gets merged into master, the failures should resolve

[steveburnett]

LGTM! (docs)

New review prompted by changes to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/sql/alter-table.rst. Pulled branch, new local build of docs, looks good. Thanks!

[pratyakshsharma]

I see clickhouse tests are disabled in the pom.xml here -

presto/presto-clickhouse/pom.xml

Line 248 in 7d19516

<include>**/TestClickHouseDistributedQueries.java</include>

.
So should I still proceed with adding the clickhouse tests in this PR? @agrawalreetika @yingsu00

As per this comment - #21894 (comment), it looks like the existing tests are failing as well

[pratyakshsharma]

Updated release notes, thank you @steveburnett

[agrawalreetika]

Overall looks good to me.
Left few commetns around adding tests, please check

[pratyakshsharma]

@agrawalreetika @yingsu00 this is good for another pass

[agrawalreetika]

LGTM

[hantangwangd]

Thanks for this work, overall looks good to me! Some nits and little things.

[ZacBlanco]

Mostly nits. Otherwise LGTM. One question though - is there any way to "unset" a table property through this in order to take up the default value instead? Or would a user have to manually look up the default value and set it?

[pratyakshsharma]

@ZacBlanco Currently there is no way to unset other than setting the default value manually.

[pratyakshsharma]

@hantangwangd @ZacBlanco @yingsu00 this is good for another pass.

[hantangwangd]

Thanks for the fix, lgtm!