Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add enforcement for invariants to commit checker #1882

Closed
wants to merge 12 commits into from

Conversation

hntd187
Copy link
Collaborator

@hntd187 hntd187 commented Nov 18, 2023

Description

Added invariant checking in the commit checker.

Related Issue(s)

#1568 closes #1880

Documentation

@github-actions github-actions bot added binding/rust Issues for the Rust crate crate/core labels Nov 18, 2023
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@hntd187 hntd187 changed the title feat: Added enforcement for invariants to commit checker Add enforcement for invariants to commit checker Nov 18, 2023
@rtyler rtyler changed the title Add enforcement for invariants to commit checker feat: add enforcement for invariants to commit checker Nov 18, 2023
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

There may have been a bit of miscommunication :).

The thing we need to decide is, if we do the invariant check or not - i.e. are invariants enabled. This is the case for 2 <= writer_version < 7 and if writer_version == 7 & 'invariants' in writer_features. And only if invariants are enabled we should inspect the schema to see if there are any invariants to validate.

One way to do this would be to make sure we only pass invariants to the DeltaDataChecker if they are enabled.

It also means we likely need to error if invariants are enabled and we have some defined, but the datafusion features and with that the checker is not available.

crates/deltalake-core/src/kernel/actions/types.rs Outdated Show resolved Hide resolved
@hntd187
Copy link
Collaborator Author

hntd187 commented Nov 19, 2023

According to the protocol invariants for 2<=>6 are always enabled, so you'd only care to check if you were 7 or higher. Are you able to disable them prior to writer feature checks?

@roeap
Copy link
Collaborator

roeap commented Nov 22, 2023

Are you able to disable them prior to writer feature checks?

No, as you said, between 2 and 6 they are always enabled. The main point is that it is not an error if they are configured on the schema, so we should not error, rather if if they are not enabled, we just kind of pretend they don't exist and do nothing w.r.t. to them ...

@github-actions github-actions bot added the binding/python Issues for the Python package label Nov 25, 2023
@hntd187
Copy link
Collaborator Author

hntd187 commented Jan 2, 2024

I added this in check constraints anyways so this PR isn't needed anymore.

@hntd187 hntd187 closed this Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/python Issues for the Python package binding/rust Issues for the Rust crate crate/core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check Invariants are respecting table features for write paths
5 participants