DDL for Table Constraints #20384

ClarenceThreepwood · 2023-07-26T01:50:14Z

Introduces table constraints to Presto (Primary Key and Unique)(#20034). Support for other types of constraints will follow.

This PR Implements

creating tables with unique and PK constraints
Adding unique and PK constraints to existing tables
Dropping existing constraints from tables

== RELEASE NOTES ==
General Changes

Add framework to add and drop table constraints (Primary Key and Unique)

Hive Connector Changes

Add support for table constraints. Other systems that support table constraints may extend this framework in their respective connectors to add support.

ZacBlanco

Just a few random comments. Main question was about the grammar for the constraint specification.

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

presto-spi/src/main/java/com/facebook/presto/spi/constraints/TableConstraint.java

presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorMetadata.java

presto-parser/src/main/java/com/facebook/presto/sql/tree/AddConstraint.java

presto-spi/src/main/java/com/facebook/presto/spi/constraints/TableConstraint.java

aaneja

LGTM overall, still taking a look at some tests/other nits.

A thought I had - IMO this may be a good time to define table constraints for the TPCH and TPCDS tables, so that we can have another sample implementation that dev's can examine and play around with.
This would be very helpful to see the impact of adding constraints to plan shapes

aaneja · 2023-08-22T04:06:07Z

presto-main/src/test/java/com/facebook/presto/execution/TestCreateTableTask.java

@@ -30,6 +30,7 @@
 import com.facebook.presto.spi.PrestoException;


Let's add a unit test similar to testCreateWithNotNullColumns that asserts that adding a new table constraint get's reflected in the ConnectorTableMetadata for supported connectors

Can you also include some negative tests

Multiple PRIMARY KEYs

Duplicate UNIQUE keys for the same named/unnamed constraint

These will be parsed by the parser, but since these are semantically incorrect, we should fail them in the CreateTableTask

I have left almost all semantic checking to the connector(s) or catalog service. It is impossible to detect all cases of semantic errors and different systems have their own rules for handling these. E.g. some systems require a globally unique name for primary keys, while others do not.
It may be possible to catch some of the common, universal errors early and return, but I'm not sure whether that is worth it. I could be convinced otherwise

I think we should have at least the logically impossible cases (such as multiple PKs) covered in presto-main. For others, lets pick a connector (say HMS) and put some negative tests there.

I've added some additional error checks here that basically validate whether the create table statement is internally consistent. I am reluctant to make these more exhaustive since they would include additional overhead of communicating with the remote catalog

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

presto-parser/src/main/java/com/facebook/presto/sql/tree/CreateTable.java

steveburnett

LGTM! (docs)

github-actions · 2023-09-11T16:33:08Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff dccb5a4...1860a8d.

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
@steveburnett	presto-docs/src/main/sphinx/language/reserved.rst presto-docs/src/main/sphinx/sql/alter-table.rst presto-docs/src/main/sphinx/sql/create-table.rst

ClarenceThreepwood · 2024-02-06T22:59:41Z

Does this PR need a release note entry added in the Description?

Added - thanks for catching this

steveburnett · 2024-02-13T20:55:27Z

Suggested revision of release note entry based on my possibly incomplete understanding of this PR and the release note guidelines:

== RELEASE NOTES ==
General Changes
* Add framework to add and drop table constraints (Primary Key and Unique)

Hive Connector Changes
* Add support for table constraints. Other systems that support table constraints may extend this framework in their respective connectors to add support.

Let me know if this accurately describes your work here!

Also and unrelated: pulled branch today and made a new local build to recheck the docs, everything looks good. Thanks for the doc!

ClarenceThreepwood · 2024-02-15T23:40:51Z

@tdcmeehan - Any other feedback?

tdcmeehan · 2024-02-17T01:27:09Z

LGTM. Any final thoughts @mlyublena @feilong-liu ?

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

steveburnett

LGTM! (docs)

Pull updated branch, new local build, everything looks good. Thanks for the doc!

Impose ordering on table columns in constraint to conform with Hive spec

Supported in the Hive connector for Hive/HMS version >= 2.1.0 ALTER TABLE DROP CONSTRAINT constraint_name

Supported in the Hive connector for Hive/HMS version >= 2.1.0 ALTER TABLE table_name ADD CONSTRAINT constraint_name UNIQUE/PRIMARY KEY (cols...)

Supported in the Hive connector for Hive/HMS version >= 2.1.0 CREATE TABLE table_name (column_definition, ..., CONSTRAINT constraint_name UNIQUE/PRIMAY KEY (column_name, ...), ...)

steveburnett

LGTM! (docs)

Pull updated branch, new local build.

ClarenceThreepwood force-pushed the pkuniqueddl branch from 60e835c to 5e11c18 Compare July 27, 2023 04:17

aaneja assigned aaneja and unassigned aaneja Jul 27, 2023

aaneja self-requested a review July 27, 2023 17:32

ClarenceThreepwood force-pushed the pkuniqueddl branch 3 times, most recently from 9791f60 to 2b6e3a7 Compare August 2, 2023 22:03

ZacBlanco reviewed Aug 15, 2023

View reviewed changes

aaneja reviewed Aug 21, 2023

View reviewed changes

aaneja reviewed Aug 22, 2023

View reviewed changes

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

aaneja reviewed Aug 22, 2023

View reviewed changes

presto-parser/src/main/java/com/facebook/presto/sql/tree/CreateTable.java Outdated Show resolved Hide resolved

ClarenceThreepwood force-pushed the pkuniqueddl branch from 2b6e3a7 to cfdc2b5 Compare August 26, 2023 05:52

ZacBlanco previously approved these changes Aug 28, 2023

View reviewed changes

ClarenceThreepwood force-pushed the pkuniqueddl branch 6 times, most recently from 7549c29 to babfdee Compare August 30, 2023 00:59

steveburnett approved these changes Aug 30, 2023

View reviewed changes

ClarenceThreepwood force-pushed the pkuniqueddl branch 3 times, most recently from 040e860 to e329df2 Compare August 31, 2023 23:50

ClarenceThreepwood marked this pull request as ready for review September 11, 2023 16:31

ClarenceThreepwood requested a review from a team as a code owner September 11, 2023 16:31

ClarenceThreepwood requested a review from presto-oss September 11, 2023 16:31

tdcmeehan self-assigned this Sep 22, 2023

ClarenceThreepwood requested a review from tdcmeehan September 22, 2023 18:10

ClarenceThreepwood force-pushed the pkuniqueddl branch from 6408333 to 20558f0 Compare February 13, 2024 20:04

tdcmeehan approved these changes Feb 20, 2024

View reviewed changes

ClarenceThreepwood force-pushed the pkuniqueddl branch from 20558f0 to be89267 Compare February 20, 2024 18:54

tdcmeehan previously approved these changes Feb 20, 2024

View reviewed changes

ClarenceThreepwood dismissed stale reviews from steveburnett, aaneja, and ZacBlanco via be89267 February 22, 2024 14:49

mlyublena reviewed Feb 23, 2024

View reviewed changes

presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved

steveburnett previously approved these changes Feb 23, 2024

View reviewed changes

ClarenceThreepwood added 6 commits February 25, 2024 19:51

Add table constraint properties "enforced" and "enabled"

1b0509d

Impose ordering on table columns in constraint to conform with Hive spec

Add DDL support for table constraints (primary key and unique - 1/3)

c3c247d

Supported in the Hive connector for Hive/HMS version >= 2.1.0 ALTER TABLE DROP CONSTRAINT constraint_name

Add DDL support for table constraints (primary key and unique - 2/3)

a99a20d

Supported in the Hive connector for Hive/HMS version >= 2.1.0 ALTER TABLE table_name ADD CONSTRAINT constraint_name UNIQUE/PRIMARY KEY (cols...)

Add DDL support for table constraints (primary key and unique - 3/3)

6a46fc4

Supported in the Hive connector for Hive/HMS version >= 2.1.0 CREATE TABLE table_name (column_definition, ..., CONSTRAINT constraint_name UNIQUE/PRIMAY KEY (column_name, ...), ...)

Add documentation for DDL for table constraints (primary and unique)

6be0e70

Disallow writes into hive tables that have any enforced constraints

1860a8d

ClarenceThreepwood dismissed stale reviews from steveburnett and tdcmeehan via 1860a8d February 26, 2024 04:34

ClarenceThreepwood force-pushed the pkuniqueddl branch from be89267 to 1860a8d Compare February 26, 2024 04:34

tdcmeehan approved these changes Feb 26, 2024

View reviewed changes

steveburnett approved these changes Feb 26, 2024

View reviewed changes

ClarenceThreepwood merged commit 492aeeb into prestodb:master Feb 26, 2024
57 checks passed

ClarenceThreepwood deleted the pkuniqueddl branch February 26, 2024 17:57

abhiseksaikia mentioned this pull request Mar 5, 2024

Revert "DDL for Table Constraints" #22092

Merged

6 tasks

ClarenceThreepwood mentioned this pull request Mar 7, 2024

Make PRIMARY a non-reserved keyword #22115

Merged

ClarenceThreepwood mentioned this pull request Apr 2, 2024

Support for NOT NULL column constraints #22064

Merged

wanglinsong mentioned this pull request May 1, 2024

Add release notes for 0.287 #22647

Merged

48 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDL for Table Constraints #20384

DDL for Table Constraints #20384

ClarenceThreepwood commented Jul 26, 2023 •

edited

Loading

ZacBlanco left a comment

aaneja left a comment

aaneja Aug 22, 2023

aaneja Aug 22, 2023

ClarenceThreepwood Aug 26, 2023

aaneja Sep 4, 2023 •

edited

Loading

ClarenceThreepwood Dec 20, 2023

steveburnett left a comment

github-actions bot commented Sep 11, 2023 •

edited

Loading

ClarenceThreepwood commented Feb 6, 2024

steveburnett commented Feb 13, 2024

ClarenceThreepwood commented Feb 15, 2024

tdcmeehan commented Feb 17, 2024

steveburnett left a comment

steveburnett left a comment

		@@ -30,6 +30,7 @@
		import com.facebook.presto.spi.PrestoException;

DDL for Table Constraints #20384

DDL for Table Constraints #20384

Conversation

ClarenceThreepwood commented Jul 26, 2023 • edited Loading

ZacBlanco left a comment

Choose a reason for hiding this comment

aaneja left a comment

Choose a reason for hiding this comment

aaneja Aug 22, 2023

Choose a reason for hiding this comment

aaneja Aug 22, 2023

Choose a reason for hiding this comment

ClarenceThreepwood Aug 26, 2023

Choose a reason for hiding this comment

aaneja Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

ClarenceThreepwood Dec 20, 2023

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 11, 2023 • edited Loading

ClarenceThreepwood commented Feb 6, 2024

steveburnett commented Feb 13, 2024

ClarenceThreepwood commented Feb 15, 2024

tdcmeehan commented Feb 17, 2024

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

ClarenceThreepwood commented Jul 26, 2023 •

edited

Loading

aaneja Sep 4, 2023 •

edited

Loading

github-actions bot commented Sep 11, 2023 •

edited

Loading