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

add @unique constraint support in schema for new predicates #8827

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

shivaji-kharse
Copy link
Contributor

@shivaji-kharse shivaji-kharse commented May 16, 2023

Partially Fixes #8827
Closes: DGRAPHCORE-206
Docs PR: dgraph-io/dgraph-docs#638

This PR adds support for uniqueness constraint using @unique directive in DQL schema. This unique directive ensures that all values of the predicate are different in a Dgraph Cluster. This completes phase 1, and enables adding a new predicate with unique directive. As part of the phase 2, we will work on adding support for unique directive for existing predicates.

Performance

Live Loader before this change on 21 million dataset took 10m54s whereas after this change took 11m02s. It did not make any significant different to non-unique predicates.

How to Use

You can now specify unique in schema as follows: email: string @unique @index(hash) @upsert .. Now, Dgraph will ensure that no mutation adds a duplicate for the predicate email.

Phase 2 [TODO]

  • check if @unique can be added to schema depending upon whether existing data has any duplicates. If the existing data has any duplicates, we do not allow adding the @unique directive and return a query that allows user to identify these UIDs.
  • If index computation is in progress, we should not allow mutations with predicates for which @unique is set
  • Fix ACL to ensure that we do not end up adding duplicate users
  • Ensure that unique constraint is not violated during Bulk loader

@dgraph-bot dgraph-bot added area/core internal mechanisms area/schema Issues related to the schema language and capabilities. go Pull requests that update Go code labels May 16, 2023
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

A few comments, this looks pretty good to start with. We need to add test for parsing the schema.

worker/mutation.go Outdated Show resolved Hide resolved
worker/schema.go Show resolved Hide resolved
worker/mutation.go Show resolved Hide resolved
protos/pb/pb.pb.go Outdated Show resolved Hide resolved
@mangalaman93 mangalaman93 changed the title add @unique support in schema [WIP] add @unique support in schema May 17, 2023
@shivaji-kharse shivaji-kharse force-pushed the shiva/unique branch 2 times, most recently from 46cbb2c to e42effd Compare May 25, 2023 16:47
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
@shivaji-kharse shivaji-kharse force-pushed the shiva/unique branch 2 times, most recently from bef7d2e to 426dcd0 Compare June 28, 2023 12:23
worker/mutation.go Outdated Show resolved Hide resolved
worker/mutation.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
schema/parse.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

We should for now, do not allow adding unique when the predicate already exists. Later on, we can support unique for existing predicate as well.

edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
worker/mutation.go Outdated Show resolved Hide resolved
worker/mutation.go Outdated Show resolved Hide resolved
worker/mutation.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me now. 3 very minor comments. Please ask other people to review.

edgraph/server.go Show resolved Hide resolved
edgraph/server.go Outdated Show resolved Hide resolved
@mangalaman93 mangalaman93 changed the title [WIP] add @unique support in schema add @unique support in schema for new predicates Aug 7, 2023
Copy link
Contributor

@harshil-goel harshil-goel left a comment

Choose a reason for hiding this comment

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

I think I will need a meeting to go through the various design decisions.

edgraph/server.go Show resolved Hide resolved
edgraph/server.go Show resolved Hide resolved
edgraph/server.go Show resolved Hide resolved
@mangalaman93 mangalaman93 changed the title add @unique support in schema for new predicates add @unique constraint support in schema for new predicates Aug 8, 2023
systest/unique_test.go Outdated Show resolved Hide resolved
systest/unique_test.go Outdated Show resolved Hide resolved
harshil-goel
harshil-goel previously approved these changes Aug 28, 2023
@mangalaman93 mangalaman93 merged commit 92c5b7a into main Aug 28, 2023
@mangalaman93 mangalaman93 deleted the shiva/unique branch August 28, 2023 12:58
@rderbier
Copy link
Member

What is the behavior if you add @unique on an existing predicate and deploy the schema?
Do we reject the schema or just ignore the unique directive?

@mangalaman93
Copy link
Member

we reject the schema and the Alter request should return an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core internal mechanisms area/schema Issues related to the schema language and capabilities. go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

6 participants