Support for NOT NULL column constraints #22064

ClarenceThreepwood · 2024-03-02T00:35:17Z

Follow up to #20384

Adds support for NOT NULL column constraints in the create table statement (for the Hive connector)
Adds support for ALTER TABLE ALTER COLUMN set/drop NOT NULL

== RELEASE NOTES ==
General Changes

Add framework to add and drop not null column constraints

Hive Connector Changes

Add support for not null constraints for tables in the Hive Metastore. Other systems that support table constraints may extend this framework in their respective connectors to add support.

github-actions · 2024-04-02T05:41:25Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff be20d29...79dc2b8.

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

steveburnett

Thanks for the docs! Looks good: a few nits, nothing else.

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

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

steveburnett

Thanks for the update! Found one final nit on formatting.

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

steveburnett

LGTM! (docs)

Pull updated branch, new local build, no concerns. Thanks for the quick revisions!

ZacBlanco

Just a few questions on the API and the grammar

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-main/src/main/java/com/facebook/presto/sql/rewrite/ShowQueriesRewrite.java

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

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

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

tdcmeehan

Mostly nits

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

tdcmeehan · 2024-05-02T16:43:08Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

+                .filter(constraint -> constraint instanceof PrimaryKeyConstraint)
+                .collect(toList());
+
+        constraints.addAll(rawConstraints.stream()


For this method, should we just use imperative code and populate various builders? I'm wondering if we should throw an error if we encounter a constraint which is not among the subclasses of TableConstraint (to prevent future errors).

Added a validate method to TableConstraintsHolder to perform this check in one place

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

...store/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastoreClient.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

rschlussel

I have a concern about the alter column support. NOT NULL constraints are being treated differently (with different syntax) compared to all other constraints, but I think we should be consistent across all our constraints.

nit: noticed some of your commit titles are too long.

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

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

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

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

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

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

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

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

Supported end-to-end in the Hive connector ALTER TABLE ALTER COLUMN SET/DROP NOT NULL

ClarenceThreepwood · 2024-05-03T22:55:12Z

Thanks @rschlussel. I've also pushed the final change you suggested

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch 3 times, most recently from e1ad2e5 to 626e051 Compare March 5, 2024 22:49

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 626e051 to ed0f14e Compare March 12, 2024 17:56

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch 8 times, most recently from a3c6f0e to 532038d Compare April 2, 2024 05:36

ClarenceThreepwood changed the title ~~Inline column constraints~~ Support for NOT NULL column constraints Apr 2, 2024

ClarenceThreepwood marked this pull request as ready for review April 2, 2024 05:40

ClarenceThreepwood requested review from steveburnett, a team and jaystarshot as code owners April 2, 2024 05:40

ClarenceThreepwood requested review from presto-oss, aaneja and ZacBlanco April 2, 2024 05:40

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 532038d to fa07125 Compare April 2, 2024 06:14

steveburnett requested changes Apr 2, 2024

View reviewed changes

presto-docs/src/main/sphinx/sql/alter-table.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/sql/alter-table.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/sql/create-table.rst Outdated Show resolved Hide resolved

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from fa07125 to f4ee95b Compare April 2, 2024 17:02

steveburnett requested changes Apr 2, 2024

View reviewed changes

presto-docs/src/main/sphinx/sql/alter-table.rst Outdated Show resolved Hide resolved

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from f4ee95b to 2715fdf Compare April 2, 2024 18:55

steveburnett previously approved these changes Apr 2, 2024

View reviewed changes

ZacBlanco reviewed Apr 9, 2024

View reviewed changes

ClarenceThreepwood dismissed steveburnett’s stale review via f36d88c April 10, 2024 19:56

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 2715fdf to f36d88c Compare April 10, 2024 19:56

aaneja reviewed Apr 11, 2024

View reviewed changes

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java Outdated Show resolved Hide resolved

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java Show resolved Hide resolved

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 9255d1b to f29cd1d Compare April 11, 2024 15:56

aaneja previously approved these changes Apr 22, 2024

View reviewed changes

ZacBlanco previously approved these changes Apr 24, 2024

View reviewed changes

ClarenceThreepwood requested review from tdcmeehan and rschlussel April 29, 2024 22:39

tdcmeehan reviewed May 2, 2024

View reviewed changes

rschlussel reviewed May 2, 2024

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java Outdated Show resolved Hide resolved

presto-spi/src/main/java/com/facebook/presto/spi/constraints/NotNullConstraint.java Show resolved Hide resolved

ClarenceThreepwood dismissed stale reviews from ZacBlanco and aaneja via bd8b3ef May 3, 2024 04:23

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch 3 times, most recently from 73f5696 to 0ac7b9b Compare May 3, 2024 05:00

rschlussel reviewed May 3, 2024

View reviewed changes

presto-spi/src/main/java/com/facebook/presto/spi/constraints/TableConstraintsHolder.java Outdated Show resolved Hide resolved

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java Outdated Show resolved Hide resolved

Remove ordering restriction on constraint specifications

a907674

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 0ac7b9b to 3d43001 Compare May 3, 2024 17:24

rschlussel previously approved these changes May 3, 2024

View reviewed changes

presto-spi/src/main/java/com/facebook/presto/spi/constraints/TableConstraintsHolder.java Outdated Show resolved Hide resolved

ClarenceThreepwood added 4 commits May 3, 2024 14:37

Support create table with not null constraints in the Hive connector

ea3775c

Add DDL support for adding or dropping NOT NULL constraints

680b943

Supported end-to-end in the Hive connector ALTER TABLE ALTER COLUMN SET/DROP NOT NULL

Add documentation for NOT NULL constraints

ed8e509

Fix toString() and hashCode() in relevant parse nodes

79dc2b8

ClarenceThreepwood dismissed rschlussel’s stale review via 79dc2b8 May 3, 2024 21:45

ClarenceThreepwood force-pushed the inlinecolumnconstraints branch from 3d43001 to 79dc2b8 Compare May 3, 2024 21:45

ClarenceThreepwood requested a review from feilong-liu as a code owner May 3, 2024 21:45

rschlussel approved these changes May 6, 2024

View reviewed changes

ClarenceThreepwood merged commit 12a6e44 into prestodb:master May 6, 2024
57 checks passed

ClarenceThreepwood deleted the inlinecolumnconstraints branch May 6, 2024 17:20

wanglinsong mentioned this pull request Jun 25, 2024

Add release notes for 0.288 #23079

Merged

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for NOT NULL column constraints #22064

Support for NOT NULL column constraints #22064

ClarenceThreepwood commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading

steveburnett left a comment

steveburnett left a comment

steveburnett left a comment

ZacBlanco left a comment

tdcmeehan left a comment

tdcmeehan May 2, 2024

ClarenceThreepwood May 3, 2024

rschlussel left a comment

ClarenceThreepwood commented May 3, 2024

Support for NOT NULL column constraints #22064

Support for NOT NULL column constraints #22064

Conversation

ClarenceThreepwood commented Mar 2, 2024 • edited Loading

github-actions bot commented Apr 2, 2024 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

ZacBlanco left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan May 2, 2024

Choose a reason for hiding this comment

ClarenceThreepwood May 3, 2024

Choose a reason for hiding this comment

rschlussel left a comment

Choose a reason for hiding this comment

ClarenceThreepwood commented May 3, 2024

ClarenceThreepwood commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading