Skip to content

Allow inserting into tables with CHECK constraint in Delta Lake #14863

Closed
ebyhr wants to merge 2 commits intomasterfrom
ebi/delta-check
Closed

Allow inserting into tables with CHECK constraint in Delta Lake #14863
ebyhr wants to merge 2 commits intomasterfrom
ebi/delta-check

Conversation

@ebyhr
Copy link
Member

@ebyhr ebyhr commented Nov 2, 2022

Description

Allow inserting into tables with CHECK constraint in Delta Lake

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

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

@cla-bot cla-bot bot added the cla-signed label Nov 2, 2022
@ebyhr ebyhr force-pushed the ebi/delta-check branch 2 times, most recently from 5d2c26d to 668de6d Compare November 2, 2022 09:14
Copy link
Member

Choose a reason for hiding this comment

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

These check constrains should be assumed to be expressed in Spark SQL, right?
I think the Delta spec conveniently omits this topic, but we can assume so for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, do we want to translate SparkSQL expression into Trino expression?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should take a shortcut here.
fortunately the check constraints are typically very limited (eg no subqueries expected), so instead of pulling a big library with a big pile of challenges, I would prefer to roll out a Limited SparkSQL Language Subset™ - parser, abstract tree and conversion to Trino form. I actually have it on my todo list.

Copy link
Member

Choose a reason for hiding this comment

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

Add javadoc explaining how these String values should be understood, i.e. what can and what cannot be a check constraint value.

I guess the intention here is "String representation of a Trino SQL scalar expression that can refer to table columns by name and produces a result coercible to boolean"

Copy link
Member

Choose a reason for hiding this comment

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

The SQL spec already has a notion of constraints, so these need to be modeled in a way consistent to what the spec describes. Have you looked at that, yet? By the way, the spec refers to various forms of constraints as "integrity constraints".

Copy link
Member

Choose a reason for hiding this comment

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

@ebyhr what about splitting this PR into SPI & Engine side and the rest? Or maybe even SPI part, then engine, then the rest?
We need Martin's attention especially for the SPI part.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I will separate this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sent #14964 for engine and SPI.

Additionally, remove redundant testWritesToTableWithCheckConstraintFails method.
@ebyhr ebyhr marked this pull request as draft November 18, 2022 05:42
@ebyhr ebyhr closed this Nov 28, 2022
@ebyhr ebyhr deleted the ebi/delta-check branch November 28, 2022 00:45
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.

3 participants