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

Cherry-pick changes required for 1.2.2 release. #4951

Merged
merged 11 commits into from
Mar 17, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Mar 17, 2020

I will not squash this PR so the log is preserved.

My changes to the bulk loader are missing because the bulk loader has a lot of changes
and not all of them have been cherry-picked into this branch. Not sure what to do about
that since I didn't make all of those changes so I am not sure exactly what commits should
be cherry-picked.


This change is Reviewable

Docs Preview: Dgraph Preview

Special predicate names (i.e using Chinese characters) need to be
wrapped around "<" and ">" brackets but the lexer does not currently
support that. This PR adds support for that syntax.

Fixes #4933
When the warning to delete and re-add the value was removed, non-list uid
predicates were not being overwritten. This fixes this by iterating over the list
and replacing theexisting postings with a copy with the operation set to a delete.

It also adds a test to prevent this from happening again.

Fixes #4879
String facet values are converted to a date time value if they match a time format.
The documentation should mention this and other type conversions.
Currently, the host is only checked when the default credentials are
used. This means using custom credentials will not work if there's no
host in the destination URI as the logic that handles this case is not
executed.

This PR changes the logic so that these checks are always executed,
regardless of the status of the credentials.
If the list is too big and is not split before writing it too disk, we could end up in the situation
described in #4733. So the posting lists should be split before writing them to disk.
A language list with the value "*:." is lexed correctly and an error
saying that if all the languages (the meaning of the character *) are
requested, the language list should only have one element.

But a language list with the value ".:*" fails with a lexing error. This
change changes the lexer so that the two lists fail with the same error.
< > brackets are needed around the type name. They were missing.
Trying to skip over the split keys sometimes skips over keys belonging
to a different split key. ReadPostingList was changed to return a nil
list when trying to read a part of a split key. All the operations on a
nil list are now a no-op. This is a fix just for this release as the
actual fix requires changes to the data format.
@martinmr martinmr force-pushed the martinmr/cp-changes-1.2.2 branch from 710d4db to 1e6334e Compare March 17, 2020 20:19
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: Got a couple of comments.

Reviewed 13 of 16 files at r1, 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @danielmai, @martinmr, @MichaelJCompton, and @pawanrawal)


posting/list.go, line 777 at r3 (raw file):

// As the list grows, existing parts might be split if they become too big.
func (l *List) Rollup() ([]*bpb.KV, error) {
	if l == nil {

Is this required? In our discussions, we decided against it.


worker/export.go, line 337 at r3 (raw file):

	x.Check2(builder.WriteString("\t"))
	// While exporting type definitions, "<" and ">" brackets must be written around
	// the name of everse predicates or Dgraph won't be able to parse the exported schema.

Why does reverse predicate show up in exported data?

Also, there's a typo here -- reverse is missing an r.

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, 3 unresolved discussions (waiting on @danielmai, @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)


posting/list.go, line 39 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

	"github.com/golang/protobuf/proto"
	"github.com/pkg/errors"

Done.


posting/list.go, line 777 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Is this required? In our discussions, we decided against it.

The one in rollup is. ReadPostingList returns a nil list so it's important that Rollup works with a nil list. This will cause the split keys to be skipped.

I did the rest of the public methods just to be safe.


worker/export.go, line 337 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Why does reverse predicate show up in exported data?

Also, there's a typo here -- reverse is missing an r.

Types can have fields that are reverse predicates.

@martinmr martinmr merged commit 3ab8fbd into release/v1.2 Mar 17, 2020
@martinmr martinmr deleted the martinmr/cp-changes-1.2.2 branch March 17, 2020 23:37
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.

3 participants