Gitlw/fix json facet #2885

gitlw · 2019-01-09T18:58:33Z

This change is

rdf/parse.go

edgraph/nquads_from_json.go

srfrog

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @golangcibot, @gitlw, @manishrjain, @codexnull, and @srfrog)

edgraph/nquads_from_json.go, line 50 at r1 (raw file):

		}

		key := fname[len(prefix):]

i dont know if we can guarantee fname wont be empty string or that if will fit the prefix.

edgraph/nquads_from_json.go, line 59 at r1 (raw file):

		case string:
			if t, err := types.ParseTime(v); err == nil {
				f.ValType = api.Facet_DATETIME

I don't think we can get rid of this datetime feature, users might be depending on it.

edgraph/nquads_from_json.go, line 66 at r1 (raw file):

		case json.Number:
			jsonNumber := facetVal.(json.Number)
			if jsonFloat, err := jsonNumber.Float64(); err == nil {

json.Number type is guaranteed to be a json number, so we only need to know if it's float or int. all numbers in json are floats, so we must use the actual value with String() to know. i think you can simplify this a bit.

manishrjain

Needs more testing.

Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on @golangcibot, @gitlw, @manishrjain, and @codexnull)

edgraph/server.go, line 666 at r2 (raw file):

			// try to interpret the value as binary first
			if _, err := facets.ValFor(f); err == nil {
				glog.Infof("facet is interpreted as binary: %+v", f)

We shouldn't be logging this.

manishrjain

A few comments. Address them, and then this is good to merge.

Reviewed 1 of 5 files at r3.
Reviewable status: 1 of 5 files reviewed, 10 unresolved discussions (waiting on @codexnull, @gitlw, and @golangcibot)

edgraph/nquads_from_json.go, line 70 at r3 (raw file):

			}
		case json.Number:
			jsonNumber := facetVal.(json.Number)

Just call it number

types/facets/utils.go, line 151 at r3 (raw file):

}

func ConvertFacetValueToBinary(key string, value interface{}, sourceFacetType api.Facet_ValType) (

Try to use shorter names. For e.g., ConvertToBinary, or even ToBinary would be sufficient here.

invocation:
facets.ToBinary(key, value, type)

Func:
ToBinary(key string, value interface{}, srcType api.Facet_ValType)

The idea is that when the func is invoked, one would understand what it is doing by looking at the name AND the args being passed to it, not just the name. So, the args being passed don't typically need to be included in the name.

types/facets/utils.go, line 154 at r3 (raw file):

	*api.Facet, error) {
	// convert facet val interface{} to binary
	sourceTypeId, err := TypeIDFor(&api.Facet{ValType: sourceFacetType})

tid, err := ? :-)

types/facets/utils.go, line 164 at r3 (raw file):

	}

	targetValueBytes, ok := targetVal.Value.([]byte)

I think this check is probably unnecessary. The Marshal above should have returned an error, if the conversion failed.

types/facets/utils.go, line 168 at r3 (raw file):

		return nil, x.Errorf("Error while marshalling types.Val into binary.")
	}
	return &api.Facet{Key: key, Value: targetValueBytes, ValType: sourceFacetType}, nil

Value: dst.Value or if that doesn't work: Value: dst.Value.([]byte)

…d to the res

gitlw

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @codexnull, @golangcibot, @manishrjain, and @srfrog)

edgraph/nquads_from_json.go, line 31 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not goimports-ed (from goimports)

Done.

edgraph/nquads_from_json.go, line 50 at r1 (raw file):

guarantee
The if block above ensures that fname has the variable prefix as its prefix, and therefore it's guaranteed, right?

edgraph/nquads_from_json.go, line 59 at r1 (raw file):

Previously, srfrog (Gus) wrote…

I don't think we can get rid of this datetime feature, users might be depending on it.

Parsing the string value as a datetime is still supported since facets.FacetFor calls valAndValType, which tries to parse the string as a datetime as one of its steps.

edgraph/nquads_from_json.go, line 66 at r1 (raw file):

Previously, srfrog (Gus) wrote…

json.Number type is guaranteed to be a json number, so we only need to know if it's float or int. all numbers in json are floats, so we must use the actual value with String() to know. i think you can simplify this a bit.

Good point, I changed the logic back to the previous way of detecting float by searching dot in the string.

edgraph/nquads_from_json.go, line 70 at r3 (raw file):