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

Change split keys to have a different prefix. #4908

Merged
merged 10 commits into from
Mar 16, 2020
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Mar 10, 2020

Currently ReadPostingList is skipping the iterator past the last possible key for
a multi-part list. The assumption was that the only keys in this range are the
main keys and the keys for each part. This is the case when a predicate has a
term and count index.

This PR changes the format of the split keys so that the first byte is different for
the keys holding parts of a multi-part list. This allows the stream framework to
skip over those keys during a rollup, eliminating the need for skipping keys.

This PR, also changes ReadPostingList to return a nil list when an individual part
is requested and the List struct so that all the operations on a nil list are a no-op
to safely handle cases where the list is being tried to accessed from the key of one
of its parts.

Fixes #4905. See that issue for more details.


This change is Reviewable

Docs Preview: Dgraph Preview

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

The split key code generation has issue. It overwrites over the count key. Please add tests.

Do not merge this change.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @manishrjain)

@martinmr martinmr requested a review from a team as a code owner March 13, 2020 00:25
@martinmr martinmr changed the title Do not skip over keys in ReadPostingList Change split keys to have a different prefix. Mar 13, 2020
Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Good to go.

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr)


posting/list.go, line 543 at r1 (raw file):

//  })
func (l *List) Iterate(readTs uint64, afterUid uint64, f func(obj *pb.Posting) error) error {
	if l == nil {

Let's remove all the l == nil.


posting/mvcc.go, line 162 at r1 (raw file):

		// public methods of the List object are no-ops and the list is being already
		// accessed via the main key in the places where this code is reached (e.g rollups).
		return nil, nil

could return an error.


x/keys.go, line 420 at r1 (raw file):

// GetSplitKey takes a key baseKey and generates the key of the list split that starts at startUid.
func GetSplitKey(baseKey []byte, startUid uint64) ([]byte, error) {

Remove the Get prefix?


x/keys.go, line 426 at r1 (raw file):

	// Change the first byte (i.e the key prefix) to ByteSplit to signal this is an
	// individual part of a single list key.
	keyCopy[0] = ByteSplit

Maybe check that it has a defaultPrefix byte, and not another byte. Return error in that case.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @manishrjain)


posting/list.go, line 543 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Let's remove all the l == nil.

Done.


posting/mvcc.go, line 162 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

could return an error.

Done.


x/keys.go, line 420 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove the Get prefix?

Done.


x/keys.go, line 426 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe check that it has a defaultPrefix byte, and not another byte. Return error in that case.

Done.

@martinmr martinmr merged commit ff06426 into master Mar 16, 2020
@martinmr martinmr deleted the martinmr/read-pl-no-skip branch March 16, 2020 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

ReadPostingList skipping over valid keys
2 participants