Skip to content

Allow writing to Delta Lake tables using writer v3#14068

Merged
ebyhr merged 1 commit intotrinodb:masterfrom
alexjo2144:delta/check-constraints
Sep 14, 2022
Merged

Allow writing to Delta Lake tables using writer v3#14068
ebyhr merged 1 commit intotrinodb:masterfrom
alexjo2144:delta/check-constraints

Conversation

@alexjo2144
Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 commented Sep 8, 2022

Description

This version includes CHECK constraints, which are not yet supported. The minimum required support is just to fail writes to tables that have these constraints.

Non-technical explanation

Allow writes to Delta tables using writer version 3. This does not yet include support for CHECK constraints.

Release notes

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

# Delta Lake
* Add support for writing to tables using writer v3. This does not yet include support for `CHECK` constraints. ({issue}`14068`)

@alexjo2144
Copy link
Copy Markdown
Member Author

Can someone kick off a run with secrets? I was not able to run the new test locally

assertQueryFailure(() -> onTrino().executeQuery("UPDATE " + tableName + " SET a = 3 WHERE b = 3"))
.hasMessageContaining("Writing to tables with CHECK constraints is not supported");
assertQueryFailure(() -> onTrino().executeQuery("DELETE FROM " + tableName + " WHERE a = 3"))
.hasMessageContaining("Writing to tables with CHECK constraints is not supported");
Copy link
Copy Markdown
Member

@homar homar Sep 8, 2022

Choose a reason for hiding this comment

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

you are missing a test for MERGE here ;)

@alexjo2144
Copy link
Copy Markdown
Member Author

Thanks, added a merge test and one to make sure that changing column comments, adding table comments, and adding columns retains the check constraint metadata.

@findepi
Copy link
Copy Markdown
Member

findepi commented Sep 12, 2022

Delta tests are failing, might be related.

@alexjo2144 alexjo2144 force-pushed the delta/check-constraints branch from 5cca6f1 to d612d89 Compare September 12, 2022 17:01
@alexjo2144
Copy link
Copy Markdown
Member Author

Had to fix some error messages. Thanks

Comment on lines +487 to +488
assertQueryFailure(() -> onTrino().executeQuery("DELETE FROM delta.default." + tableName + " WHERE a = 3"))
.hasMessageContaining("Writing to tables with CHECK constraints is not supported");
Copy link
Copy Markdown
Member

@ebyhr ebyhr Sep 13, 2022

Choose a reason for hiding this comment

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

Why do we want to disable DELETE when a table has check constraints? Is there any situation deleting rows violate check constraints?

I'm fine with disallowing it because allowing only DELETE looks not useful. Just asking to understand the context.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It should be safe to enable as long as the check constraints are only row by row. I think I'd opt to leave it disabled for now though and we can enable it when we come back and take a closer look at supporting check constraints.

This version includes CHECK constraints, which are not yet supported.
@alexjo2144 alexjo2144 force-pushed the delta/check-constraints branch from d612d89 to 6fdca7c Compare September 13, 2022 15:35
@alexjo2144
Copy link
Copy Markdown
Member Author

Renamed those two tests, thanks @ebyhr

@ebyhr ebyhr merged commit 508c70c into trinodb:master Sep 14, 2022
@ebyhr
Copy link
Copy Markdown
Member

ebyhr commented Sep 14, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 396 milestone Sep 14, 2022
@ebyhr ebyhr mentioned this pull request Sep 14, 2022
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.

4 participants