Skip to content

Commit

Permalink
fix: Remove limit for fetching secondary docs (#2594)
Browse files Browse the repository at this point in the history
## Relevant issue(s)

Resolves #2590 and #2600

## Description

Make join fetch all secondary docs of a fetched-by-index primary doc.
  • Loading branch information
islamaliev authored and shahzadlone committed May 14, 2024
1 parent 592c035 commit 73ad077
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 11 deletions.
9 changes: 2 additions & 7 deletions internal/planner/type_join.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,15 +294,15 @@ func (n *typeJoinOne) Kind() string {
return "typeJoinOne"
}

func fetchDocsWithFieldValue(plan planNode, fieldName string, val any, limit uint) ([]core.Doc, error) {
func fetchDocsWithFieldValue(plan planNode, fieldName string, val any) ([]core.Doc, error) {
propIndex := plan.DocumentMap().FirstIndexOfName(fieldName)
setSubTypeFilterToScanNode(plan, propIndex, val)

if err := plan.Init(); err != nil {
return nil, NewErrSubTypeInit(err)
}

docs := make([]core.Doc, 0, limit)
var docs []core.Doc
for {
next, err := plan.Next()
if err != nil {
Expand All @@ -313,10 +313,6 @@ func fetchDocsWithFieldValue(plan planNode, fieldName string, val any, limit uin
}

docs = append(docs, plan.Value())

if limit > 0 && len(docs) >= int(limit) {
break
}
}

return docs, nil
Expand Down Expand Up @@ -587,7 +583,6 @@ func (join *invertibleTypeJoin) Next() (bool, error) {
// otherwise the user would not have been able to request it.
join.dir.secondaryField.Value(),
firstDoc.GetID(),
join.secondaryFetchLimit,
)
if err != nil {
return false, err
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/index/docs.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func getUserDocs() predefined.DocsList {
},
{
"model": "Playstation 5",
"year": 2022,
"year": 2021,
"type": "game_console",
"specs": map[string]any{
"CPU": 3.5,
Expand Down
134 changes: 131 additions & 3 deletions tests/integration/index/query_with_relation_filter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,11 @@ func TestQueryWithIndexOnOneToOnePrimaryRelation_IfFilterOnIndexedFieldOfRelatio
},
},
testUtils.Request{
Request: makeExplainQuery(req1),
Request: makeExplainQuery(req1),
// we make 1 index fetch to get the only address with city == "London"
// then we scan all 10 users to find one with matching "address_id"
// after this we fetch the name of the user
// it should be optimized after this is done https://github.com/sourcenetwork/defradb/issues/2601
Asserter: testUtils.NewExplainAsserter().WithFieldFetches(11).WithIndexFetches(1),
},
testUtils.Request{
Expand All @@ -264,8 +268,12 @@ func TestQueryWithIndexOnOneToOnePrimaryRelation_IfFilterOnIndexedFieldOfRelatio
},
},
testUtils.Request{
Request: makeExplainQuery(req2),
Asserter: testUtils.NewExplainAsserter().WithFieldFetches(15).WithIndexFetches(3),
Request: makeExplainQuery(req2),
// we make 3 index fetch to get the 3 address with city == "Montreal"
// then we scan all 10 users to find one with matching "address_id" for each address
// after this we fetch the name of each user
// it should be optimized after this is done https://github.com/sourcenetwork/defradb/issues/2601
Asserter: testUtils.NewExplainAsserter().WithFieldFetches(33).WithIndexFetches(3),
},
},
}
Expand Down Expand Up @@ -553,3 +561,123 @@ func TestQueryWithIndexOnOneToOne_IfFilterOnIndexedRelation_ShouldFilter(t *test

testUtils.ExecuteTestCase(t, test)
}

func TestQueryWithIndexOnManyToOne_IfFilterOnIndexedField_ShouldFilterWithExplain(t *testing.T) {
// This query will fetch first a matching device which is secondary doc and therefore
// has a reference to the primary User doc.
req := `query {
Device(filter: {
year: {_eq: 2021}
}) {
model
owner {
name
}
}
}`
test := testUtils.TestCase{
Description: "With filter on indexed field of secondary relation (N-1) should fetch secondary and primary objects",
Actions: []any{
testUtils.SchemaUpdate{
Schema: `
type User {
name: String
devices: [Device]
}
type Device {
model: String
year: Int @index
owner: User
}
`,
},
testUtils.CreatePredefinedDocs{
Docs: getUserDocs(),
},
testUtils.Request{
Request: req,
Results: []map[string]any{
{
"model": "Playstation 5",
"owner": map[string]any{
"name": "Islam",
},
},
{
"model": "Playstation 5",
"owner": map[string]any{
"name": "Addo",
},
},
{
"model": "iPhone 10",
"owner": map[string]any{
"name": "Addo",
},
},
},
},
testUtils.Request{
Request: makeExplainQuery(req),
// we make 3 index fetches to get all 3 devices with year 2021
// and 9 field fetches: for every device we fetch additionally "model", "owner_id" and owner's "name"
Asserter: testUtils.NewExplainAsserter().WithFieldFetches(9).WithIndexFetches(3),
},
},
}

testUtils.ExecuteTestCase(t, test)
}

func TestQueryWithIndexOnManyToOne_IfFilterOnIndexedRelation_ShouldFilterWithExplain(t *testing.T) {
// This query will fetch first a matching user (owner) which is primary doc and therefore
// has no direct reference to secondary Device docs.
// At the moment the db has to make a full scan of the Device docs to find the matching ones.
// Keenan has 3 devices.
req := `query {
Device(filter: {
owner: {name: {_eq: "Keenan"}}
}) {
model
}
}`
test := testUtils.TestCase{
Description: "Upon querying secondary object with filter on indexed field of primary relation (in 1-N) should fetch all secondary objects of the same primary one",
Actions: []any{
testUtils.SchemaUpdate{
Schema: `
type User {
name: String @index
devices: [Device]
}
type Device {
model: String
owner: User
}
`,
},
testUtils.CreatePredefinedDocs{
Docs: getUserDocs(),
},
testUtils.Request{
Request: req,
Results: []map[string]any{
{"model": "iPhone 13"},
{"model": "iPad Mini"},
{"model": "MacBook Pro"},
},
},
testUtils.Request{
Request: makeExplainQuery(req),
// we make only 1 index fetch to get the owner by it's name
// and 44 field fetches to get 2 fields for all 22 devices in the db.
// it should be optimized after this is done https://github.com/sourcenetwork/defradb/issues/2601
Asserter: testUtils.NewExplainAsserter().WithFieldFetches(44).WithIndexFetches(1),
},
},
}

testUtils.ExecuteTestCase(t, test)
}
18 changes: 18 additions & 0 deletions tests/integration/test_case.go
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,24 @@ type GenerateDocs struct {
}

// CreatePredefinedDocs is an action that will trigger creation of predefined documents.
// Predefined docs allows specifying a database state with complex schemas that can be used by
// multiple tests while allowing each test to select a subset of the schemas (collection and
// collection's fields) to work with.
// Example:
//
// gen.DocsList{
// ColName: "User",
// Docs: []map[string]any{
// {
// "name": "Shahzad",
// "devices": []map[string]any{
// {
// "model": "iPhone Xs",
// }},
// }},
// }
//
// For more information refer to tests/predefined/README.md
type CreatePredefinedDocs struct {
// NodeID may hold the ID (index) of a node to execute the generation on.
//
Expand Down
3 changes: 3 additions & 0 deletions tests/predefined/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ gen.DocsList{
"year": 2022,
"type": "phone",
}},
"address": map[string]any{
"city": "Munich",
},
}},
}
```
Expand Down

0 comments on commit 73ad077

Please sign in to comment.