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

Validate and converting facets to binary #2797

Merged
merged 3 commits into from
Dec 5, 2018

Conversation

gitlw
Copy link

@gitlw gitlw commented Nov 29, 2018

  • Removed the TryValFor function
  • Simplified TypeIDFor and ValFor
  • Converting facets to binary format

This change is Reviewable

- Simplified TypeIDFor and ValFor
- Converting facets to binary format
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.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gitlw, @danielmai, @codexnull, and @srfrog)


edgraph/server.go, line 633 at r1 (raw file):

	/* parse facets and convert to the binary format so that
	a field of type datetime like "2017-01-01" can be correctly encoded in the

use line comments. https://golang.org/doc/effective_go.html#commentary


edgraph/server.go, line 650 at r1 (raw file):

func validateAndConvertFacets(mu *api.Mutation) error {
	for _, m := range mu.Set {

What about mu.Del? If they're trying to delete facets (assuming that's where deletions go, i.e. in mu.Del)?


types/facets/utils.go, line 178 at r1 (raw file):

// TypeIDFor gives TypeID for facet.
func TypeIDFor(f *api.Facet) types.TypeID {
	switch TypeIDForValType(f.ValType) {

We can probably remove TypeIDForValType function. I doubt it's being used anymore.


worker/export.go, line 107 at r1 (raw file):

				fVal, err := facets.ValFor(f)
				if err != nil {
					return err

Just log it, don't return err.


worker/export.go, line 112 at r1 (raw file):

				fStringVal := &types.Val{Tid: types.StringID}
				if err = types.Marshal(fVal, fStringVal); err != nil {
					return err

log it, don't return.


worker/export.go, line 116 at r1 (raw file):

				facetTid, err := facets.TypeIDFor(f)
				if err != nil {
					return err

same here.

@gitlw gitlw force-pushed the gitlw/convert_facets_to_binary branch from 3672208 to c95197b Compare December 4, 2018 22:28
Copy link
Author

@gitlw gitlw 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: 5 of 9 files reviewed, 6 unresolved discussions (waiting on @manishrjain, @danielmai, @codexnull, and @srfrog)


edgraph/server.go, line 633 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

use line comments. https://golang.org/doc/effective_go.html#commentary

Done.


edgraph/server.go, line 650 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

What about mu.Del? If they're trying to delete facets (assuming that's where deletions go, i.e. in mu.Del)?

It seems there is no way to delete a facet directly with mu.Del.
Supposedly a facet is removed by doing a rewrite on the predicate with a new facet set excluding the one to be deleted.


types/facets/utils.go, line 178 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We can probably remove TypeIDForValType function. I doubt it's being used anymore.

Done.


worker/export.go, line 107 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Just log it, don't return err.

Done.


worker/export.go, line 112 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

log it, don't return.

Done.


worker/export.go, line 116 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

same here.

Done.

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: Thanks for fixing this!

Reviewable status: 5 of 9 files reviewed, all discussions resolved (waiting on @manishrjain, @danielmai, @codexnull, and @srfrog)

@gitlw gitlw merged commit a25d2b7 into master Dec 5, 2018
@gitlw gitlw deleted the gitlw/convert_facets_to_binary branch December 5, 2018 17:58
@danielmai
Copy link
Contributor

This PR addressed this discuss topic: https://discuss.dgraph.io/t/how-to-add-date-type-facet-by-using-dgraph4j/3639

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
- Removed the TryValFor function
- Simplified TypeIDFor and ValFor
- Converting facets to binary format
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