Add ALTER TABLE SET PROPERTIES statement #21495

pratyakshsharma · 2023-12-07T04:04:47Z

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 · 2023-12-07T04:05:42Z

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 · 2023-12-07T04:37:20Z

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 · 2023-12-25T18:10:30Z

@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 · 2023-12-26T05:15:49Z

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

pratyakshsharma · 2023-12-27T17:00:09Z

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

pratyakshsharma · 2023-12-28T11:40:53Z

This is ready to be reviewed.

presto-base-jdbc/src/main/java/com/facebook/presto/plugin/jdbc/JdbcClient.java

agrawalreetika · 2024-01-03T18:56:05Z

presto-main/src/main/java/com/facebook/presto/metadata/AbstractPropertyManager.java

+            Map<String, Expression> sqlPropertyValues,
+            Session session,
+            Metadata metadata,
+            Map<NodeRef<Parameter>, Expression> parameters)


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

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

agrawalreetika · 2024-01-03T19:00:34Z

presto-clickhouse/src/main/java/com/facebook/presto/plugin/clickhouse/ClickHouseMetadata.java

+        ClickHouseTableHandle tableHandle = (ClickHouseTableHandle) table;
+        clickHouseClient.setTableProperties(ClickHouseIdentity.from(session), tableHandle, properties);
+    }
+


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

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 · 2024-01-03T19:16:50Z

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

pratyakshsharma · 2024-01-06T17:14:24Z

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 · 2024-02-12T15:16:31Z

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

steveburnett

LGTM! (docs)

Pull branch, local doc build. Thanks!

steveburnett · 2024-09-26T14:03:15Z

Nit to add PR number to release note entry.
Also suggest add a link to the ALTER TABLE documentation. I tested this link example in a local doc build.

== RELEASE NOTES ==

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

pratyakshsharma · 2024-09-26T15:07:54Z

@yingsu00 this is good to take a pass.

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java

pratyakshsharma · 2024-10-04T14:15:17Z

Updated release notes, thank you @steveburnett

agrawalreetika

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

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java

pratyakshsharma · 2024-11-06T12:08:32Z

@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.

presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java

presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java

presto-docs/src/main/sphinx/sql/alter-table.rst

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

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?

presto-main/src/main/java/com/facebook/presto/execution/SetPropertiesTask.java

presto-main/src/main/java/com/facebook/presto/execution/SetSessionTask.java

presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java

presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java

pratyakshsharma · 2024-11-18T15:30:59Z

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

Cherry-pick of trinodb/trino@72df2b9 Co-authored-by: ebyhr

pratyakshsharma · 2024-11-20T08:37:13Z

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

hantangwangd

Thanks for the fix, lgtm!

pratyakshsharma requested a review from a team as a code owner December 7, 2023 04:04

pratyakshsharma requested a review from presto-oss December 7, 2023 04:04

steveburnett approved these changes Dec 7, 2023

View reviewed changes

pratyakshsharma force-pushed the alter-tbl-properties-support branch from f1bc667 to a4332d9 Compare December 7, 2023 18:27

pratyakshsharma marked this pull request as draft December 7, 2023 18:28

pratyakshsharma force-pushed the alter-tbl-properties-support branch from a4332d9 to 787eb0b Compare December 25, 2023 18:03

pratyakshsharma marked this pull request as ready for review December 25, 2023 18:04

pratyakshsharma force-pushed the alter-tbl-properties-support branch 2 times, most recently from 577174d to 4d21e4b Compare December 25, 2023 19:54

pratyakshsharma mentioned this pull request Dec 27, 2023

Add support for iceberg set properties #21606

Closed

6 tasks

pratyakshsharma force-pushed the alter-tbl-properties-support branch from 4d21e4b to 0efcfd1 Compare December 27, 2023 16:14

pratyakshsharma force-pushed the alter-tbl-properties-support branch from 4d92356 to f1bd1d4 Compare December 28, 2023 08:40

agrawalreetika requested changes Jan 3, 2024

View reviewed changes

pratyakshsharma force-pushed the alter-tbl-properties-support branch 3 times, most recently from 235c533 to 3853b50 Compare January 6, 2024 13:20

pratyakshsharma force-pushed the alter-tbl-properties-support branch from 3853b50 to dbabd11 Compare February 9, 2024 16:25

steveburnett previously approved these changes Feb 9, 2024

View reviewed changes

pratyakshsharma force-pushed the alter-tbl-properties-support branch 3 times, most recently from 5517bd7 to 7b234e9 Compare February 14, 2024 12:58

pratyakshsharma requested review from ZacBlanco and ClarenceThreepwood as code owners September 26, 2024 13:39

steveburnett previously approved these changes Sep 26, 2024

View reviewed changes

agrawalreetika requested changes Sep 26, 2024

View reviewed changes

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/metadata/MetadataUtil.java Show resolved Hide resolved

yingsu00 self-assigned this Sep 27, 2024

pratyakshsharma dismissed steveburnett’s stale review via d200ddc October 4, 2024 14:12

pratyakshsharma force-pushed the alter-tbl-properties-support branch from 338b6ea to d200ddc Compare October 4, 2024 14:12

agrawalreetika requested changes Oct 28, 2024

View reviewed changes

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java Show resolved Hide resolved

presto-parser/src/test/java/com/facebook/presto/sql/parser/TestStatementBuilder.java Show resolved Hide resolved

pratyakshsharma force-pushed the alter-tbl-properties-support branch from d200ddc to b7734dd Compare October 28, 2024 12:44

pratyakshsharma force-pushed the alter-tbl-properties-support branch from b7734dd to 76687fe Compare November 6, 2024 10:57

pratyakshsharma force-pushed the alter-tbl-properties-support branch 2 times, most recently from 0831752 to deaa9e8 Compare November 13, 2024 07:22

agrawalreetika previously approved these changes Nov 13, 2024

View reviewed changes

pratyakshsharma requested a review from yingsu00 November 13, 2024 11:48

hantangwangd reviewed Nov 13, 2024

View reviewed changes

ZacBlanco reviewed Nov 14, 2024

View reviewed changes

pratyakshsharma added 3 commits November 19, 2024 16:28

Add ALTER TABLE SET PROPERTIES statement

0654e2f

Cherry-pick of trinodb/trino@72df2b9 Co-authored-by: ebyhr

Refactor and create new method in MetadataUtil

bb25355

Add set properties support for iceberg

4f826af

pratyakshsharma dismissed agrawalreetika’s stale review via 4f826af November 19, 2024 11:06

pratyakshsharma force-pushed the alter-tbl-properties-support branch from deaa9e8 to 4f826af Compare November 19, 2024 11:06

hantangwangd approved these changes Nov 20, 2024

View reviewed changes

yingsu00 approved these changes Nov 22, 2024

View reviewed changes

yingsu00 merged commit b19167e into prestodb:master Nov 22, 2024
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ALTER TABLE SET PROPERTIES statement #21495

Add ALTER TABLE SET PROPERTIES statement #21495

pratyakshsharma commented Dec 7, 2023 •

edited

Loading

github-actions bot commented Dec 7, 2023 •

edited

Loading

Akanksha-kedia commented Dec 7, 2023 •

edited

Loading

steveburnett left a comment

pratyakshsharma commented Dec 25, 2023 •

edited

Loading

pratyakshsharma commented Dec 26, 2023

pratyakshsharma commented Dec 27, 2023

pratyakshsharma commented Dec 28, 2023

agrawalreetika Jan 3, 2024

pratyakshsharma Jan 6, 2024

agrawalreetika Jan 3, 2024 •

edited

Loading

pratyakshsharma Jan 6, 2024

agrawalreetika commented Jan 3, 2024

pratyakshsharma commented Jan 6, 2024

steveburnett left a comment

pratyakshsharma commented Feb 12, 2024 •

edited

Loading

steveburnett left a comment

steveburnett commented Sep 26, 2024

pratyakshsharma commented Sep 26, 2024

pratyakshsharma commented Oct 4, 2024

agrawalreetika left a comment

pratyakshsharma commented Nov 6, 2024

agrawalreetika left a comment

hantangwangd left a comment

ZacBlanco left a comment

pratyakshsharma commented Nov 18, 2024

pratyakshsharma commented Nov 20, 2024

hantangwangd left a comment

Add ALTER TABLE SET PROPERTIES statement #21495

Add ALTER TABLE SET PROPERTIES statement #21495

Conversation

pratyakshsharma commented Dec 7, 2023 • edited Loading

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

github-actions bot commented Dec 7, 2023 • edited Loading

Akanksha-kedia commented Dec 7, 2023 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Dec 25, 2023 • edited Loading

pratyakshsharma commented Dec 26, 2023

pratyakshsharma commented Dec 27, 2023

pratyakshsharma commented Dec 28, 2023

agrawalreetika Jan 3, 2024

Choose a reason for hiding this comment

pratyakshsharma Jan 6, 2024

Choose a reason for hiding this comment

agrawalreetika Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

pratyakshsharma Jan 6, 2024

Choose a reason for hiding this comment

agrawalreetika commented Jan 3, 2024

pratyakshsharma commented Jan 6, 2024

steveburnett left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Feb 12, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett commented Sep 26, 2024

pratyakshsharma commented Sep 26, 2024

pratyakshsharma commented Oct 4, 2024

agrawalreetika left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Nov 6, 2024

agrawalreetika left a comment

Choose a reason for hiding this comment

hantangwangd left a comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Nov 18, 2024

pratyakshsharma commented Nov 20, 2024

hantangwangd left a comment

Choose a reason for hiding this comment

pratyakshsharma commented Dec 7, 2023 •

edited

Loading

github-actions bot commented Dec 7, 2023 •

edited

Loading

Akanksha-kedia commented Dec 7, 2023 •

edited

Loading

pratyakshsharma commented Dec 25, 2023 •

edited

Loading

agrawalreetika Jan 3, 2024 •

edited

Loading

pratyakshsharma commented Feb 12, 2024 •

edited

Loading