[BREAKING] New Facets format #5424

ashish-goswami · 2020-05-12T05:40:57Z

Fixes: #4798, #4581, #4907
DGRAPH-1109, DGRAPH-1062, DGRAPH-1143

This is PR changes facets format as discussed in the post: https://discuss.dgraph.io/t/facets-format-in-mutation-requests-and-query-responses/6416

After this PR is merged response/requests formats will look like as below:
Current UID predicate facets query response:

{
  "data": {
    "q": [
      {
        "name": "San Francisco",
        "state": {
          "name": "California"
        },
        "state|capital": false
      }
    ]
  }
}

New UID predicate facets query response:

{
  "data": {
    "q": [
      {
        "name": "San Francisco",
        "state": {
          "name": "California",
          "state|capital": false
        }
      }
    ]
  }
}

Current UID list predicate facets query response:

{
  "data": {
    "q": [
      {
        "name": "Alice",
        "speaks": [
          {
            "name": "Spanish"
          },
          {
            "name": "Chinese"
          }
        ],
        "speaks|fluent": {
          "0": true,
          "1": false
        }
      }
    ]
  }
}

New UID list predicate facets query response:

{
  "data": {
    "q": [
      {
        "name": "Alice",
        "speaks": [
          {
            "name": "Spanish",
            "speaks|fluent": true
          },
          {
            "name": "Chinese",
            "speaks|fluent": false
          }
        ]
      }
    ]
  }
}

Current scalar list predicate facets mutation request:

{
  "set": [
    {
      "uid": "_:1",
      "nickname": "Joshua",
      "nickname|kind": "official"
    },
    {
      "uid": "_:1",
      "nickname": "David"
    },
    {
      "uid": "_:1",
      "nickname": "Josh",
      "nickname|kind": "friends"
    }
  ]
}

New scalar list predicate facets mutation request:

{
  "set": {
      "uid": "_:1",
      "nickname": ["Joshua", "David", "Josh"],
      "nickname|kind": {
         "0": "official",
         "2": "friends"
      }
   }
}

NOTE: there is no change in the request/response facets format for scalar predicate type.

This change is

Docs Preview:

pawanrawal

Please add a test for storing facets for scalar list and then confirming that a follow on query returns result in the same format in systest or http_test.go.

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ashish-goswami and @manishrjain)

chunker/json_parser.go, line 49 at r1 (raw file):

}

func handleBasicFacetsType(fname, prefix string, facetVal interface{}) (*api.Facet, error) {

Mention that this is only called when fname has prefix and from where all this is called.

chunker/json_parser.go, line 50 at r1 (raw file):

func handleBasicFacetsType(fname, prefix string, facetVal interface{}) (*api.Facet, error) {
	key := fname[len(prefix):]

Can we just pass this to the function instead of passing fname.

chunker/json_parser.go, line 103 at r1 (raw file):

// parseMapFacets parses facets which are of map type. Facets for scalar list predicates are
// specifed in map format. For example below prdicate nickname and kind facet associated with it.

specified
predicate

chunker/json_parser.go, line 112 at r1 (raw file):

// 		}
// }
// Parsed response would a slice of maps[int]*api.Facet, one map for each facet json.

one map for each facet

chunker/json_parser.go, line 132 at r1 (raw file):

		if !ok {
			return nil,
				errors.Errorf("facets format should be of type map for scalar list predicates")

errors.Errorf("facets format should be of type map for scalar list predicates, found: %v for facet: %v", facetVal, fname)

chunker/json_parser.go, line 139 at r1 (raw file):

			facet, err := handleBasicFacetsType(fname, prefix, val)
			if err != nil {
				return nil, errors.Wrapf(err, "Index: %s", sidx)

index

chunker/json_parser.go, line 143 at r1 (raw file):

			idx, err := strconv.Atoi(sidx)
			if err != nil {
				return nil, errors.Wrapf(err, "Index: %s", sidx)

index

chunker/json_parser.go, line 455 at r1 (raw file):

		// TODO - Maybe do an initial pass and build facets for all predicates. Then we don't have
		// to call parseFacets everytime.
		// Only call parseBasicFacets when value type is not list.

value type is scalar but not a list

chunker/json_parser.go, line 520 at r1 (raw file):

						return mr, err
					}
					// Also populate facets for it.

Populate all the facets for it by seeking the appropriate index across all entries in the map. Please add some comments here about example values.

chunker/json_parser_test.go, line 555 at r1 (raw file):

		{
			"name":"Alice",
			"friend": ["Joshua", "David", "Josh"],

Test with friend being null but facets being there. friend: null
Friend being empty, friend: [] but facets being there
Facet map being null but friends being there.
Facet map being of wrong type ["school", "college"] but friends being there
Facet map having index > len(friend)

pawanrawal

Once comments are addressed

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ashish-goswami and @manishrjain)

ashish-goswami

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ashish-goswami, @manishrjain, and @pawanrawal)

chunker/json_parser.go, line 50 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can we just pass this to the function instead of passing fname.

Done.

chunker/json_parser.go, line 103 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

specified
predicate

Done.

chunker/json_parser.go, line 112 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

one map for each facet

Done.

chunker/json_parser.go, line 132 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

errors.Errorf("facets format should be of type map for scalar list predicates, found: %v for facet: %v", facetVal, fname)

Done.

chunker/json_parser.go, line 139 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

index

Done.

chunker/json_parser.go, line 143 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

index

Done.

chunker/json_parser.go, line 455 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

value type is scalar but not a list

Done.

chunker/json_parser.go, line 520 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Populate all the facets for it by seeking the appropriate index across all entries in the map. Please add some comments here about example values.

Done.

chunker/json_parser_test.go, line 555 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Test with friend being null but facets being there. friend: null

Friend being empty, friend: [] but facets being there

Facet map being null but friends being there.

Facet map being of wrong type ["school", "college"] but friends being there

Facet map having index > len(friend)

Done.

gja

Small renames, but approved

chunker/json_parser.go

gja · 2020-05-13T09:57:02Z

chunker/json_parser.go

@@ -475,7 +574,7 @@ func (buf *NQuadBuffer) mapToNquads(m map[string]interface{}, op int, parentPred
 		}
 	}

-	fts, err := parseFacetsJSON(m, parentPred+x.FacetDelimeter)
+	fts, err := parseBasicFacets(m, parentPred+x.FacetDelimeter)


parseScalarFacets

manishrjain

Reviewed 5 of 8 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ashish-goswami and @pawanrawal)

chunker/json_parser.go, line 49 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Mention that this is only called when fname has prefix and from where all this is called.

Not sure what's the point of "Basic". What is not basic?

chunker/json_parser.go, line 154 at r2 (raw file):

// parseBasicFacets parses facets which should be of type string/json.Number/bool.
// It returns []*api.Facet, one *api.Facet for each facet.
func parseBasicFacets(m map[string]interface{}, prefix string) ([]*api.Facet, error) {

same here. Basic?

This reverts commit ae8af64.

ashish-goswami

Reviewable status: 6 of 8 files reviewed, 13 unresolved discussions (waiting on @ashish-goswami, @gja, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

chunker/json_parser.go, line 49 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Not sure what's the point of "Basic". What is not basic?

I have written a comment explaining what handleBasicFacetsType does.

chunker/json_parser.go, line 88 at r2 (raw file):

Previously, gja (Tejas Dinkar) wrote…

not float64, it should be number

Done.

chunker/json_parser.go, line 154 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

same here. Basic?

renamed it to parseScalarFacets

Fixes: dgraph-io#4798, dgraph-io#4581, dgraph-io#4907 DGRAPH-1109, DGRAPH-1062, DGRAPH-1143 This is PR changes facets format as discussed in the post: https://discuss.dgraph.io/t/facets-format-in-mutation-requests-and-query-responses/6416 After this PR is merged response/requests formats will look like as below: Current UID predicate facets query response: { "data": { "q": [ { "name": "San Francisco", "state": { "name": "California" }, "state|capital": false } ] } } New UID predicate facets query response: { "data": { "q": [ { "name": "San Francisco", "state": { "name": "California", "state|capital": false } } ] } } Current UID list predicate facets query response: { "data": { "q": [ { "name": "Alice", "speaks": [ { "name": "Spanish" }, { "name": "Chinese" } ], "speaks|fluent": { "0": true, "1": false } } ] } } New UID list predicate facets query response: { "data": { "q": [ { "name": "Alice", "speaks": [ { "name": "Spanish", "speaks|fluent": true }, { "name": "Chinese", "speaks|fluent": false } ] } ] } } Current scalar list predicate facets mutation request: { "set": [ { "uid": "_:1", "nickname": "Joshua", "nickname|kind": "official" }, { "uid": "_:1", "nickname": "David" }, { "uid": "_:1", "nickname": "Josh", "nickname|kind": "friends" } ] } New scalar list predicate facets mutation request: { "set": { "uid": "_:1", "nickname": ["Joshua", "David", "Josh"], "nickname|kind": { "0": "official", "2": "friends" } } } NOTE: there is no change in the request/response facets format for scalar predicate type.

dpkirchner · 2021-01-07T18:10:41Z

It looks like the format for mutating uid lists with facets was not changed -- is that correct and is it intentional? A mutation that creates facets on uid lists and on scalar lists will look like this?

uid_list: [uid] .
scalar_list: [string] .

{
  "set": {
    "uid_list": [{
      "uid": "0xdeadbeef",
      "uid_list|facet_name": 123
    }],
    "scalar_list": [
      "scalar_1"
    ],
    "scalar_list|facet_name": {
      "0": 123
    }
  }
}

ashish-goswami added 4 commits May 11, 2020 10:56

Change response format for UID/UID list type

17057ba

Fix broken tests

981e1f9

Support for facets maps for scalar list in mutation requests

c883f89

Add test for json parsing

c238480

ashish-goswami requested review from manishrjain, pawanrawal and a team as code owners May 12, 2020 05:40

Minor changes

ae108e8

pawanrawal suggested changes May 12, 2020

View reviewed changes

pawanrawal approved these changes May 12, 2020

View reviewed changes

Address review comments

96042e5

ashish-goswami commented May 13, 2020

View reviewed changes

gja approved these changes May 13, 2020

View reviewed changes

manishrjain approved these changes May 13, 2020

View reviewed changes

ashish-goswami added 2 commits May 18, 2020 19:08

Merge remote-tracking branch 'origin/master' into ashish/newfacets

eb47fea

Address review comments

ae8af64

ashish-goswami requested a review from vvbalaji-dgraph as a code owner May 18, 2020 13:45

ashish-goswami added 2 commits May 18, 2020 19:23

Revert "Address review comments"

eb40c87

This reverts commit ae8af64.

Address review comments

42ff1e8

ashish-goswami commented May 18, 2020

View reviewed changes

ashish-goswami changed the title ~~New Facets format~~ [BREAKING] New Facets format May 19, 2020

Merge remote-tracking branch 'origin/master' into ashish/newfacets

108578d

ashish-goswami merged commit b16b611 into master May 20, 2020

ashish-goswami deleted the ashish/newfacets branch May 20, 2020 13:09

This was referenced May 20, 2020

Make the new Facet response Optional. #4907

Closed

JSON mutation, Support facets for value edge lists #4581

Closed

danielmai mentioned this pull request Feb 18, 2021

fix(Mutation): Deeply-nested uid facets #7455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] New Facets format #5424

[BREAKING] New Facets format #5424

ashish-goswami commented May 12, 2020 •

edited by dgraph-bot

Loading

pawanrawal left a comment

pawanrawal left a comment

ashish-goswami left a comment

gja left a comment

gja May 13, 2020

manishrjain left a comment

ashish-goswami left a comment

dpkirchner commented Jan 7, 2021 •

edited

Loading

[BREAKING] New Facets format #5424

[BREAKING] New Facets format #5424

Conversation

ashish-goswami commented May 12, 2020 • edited by dgraph-bot Loading

pawanrawal left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

gja left a comment

Choose a reason for hiding this comment

gja May 13, 2020

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

dpkirchner commented Jan 7, 2021 • edited Loading

ashish-goswami commented May 12, 2020 •

edited by dgraph-bot

Loading

dpkirchner commented Jan 7, 2021 •

edited

Loading