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.

Sign up for GitHub

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

fix: Reset scan node for each join #828

Merged
merged 1 commit into from
Sep 19, 2022
Merged

fix: Reset scan node for each join #828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions query/graphql/planner/type_join.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,13 @@ func (n *typeJoinOne) valuesSecondary(doc core.Doc) core.Doc {
filter := map[connor.FilterKey]any{
fkIndex: doc.GetKey(),
}

// We have to reset the scan node after appending the new key-filter
if err := n.subType.Init(); err != nil {
log.ErrorE(n.p.ctx, "Sub-type initialization error at scan node reset", err)
return doc
}

// using the doc._key as a filter
err := appendFilterToScanNode(n.subType, filter)
if err != nil {
Expand Down
62 changes: 62 additions & 0 deletions tests/integration/query/one_to_one/simple_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,65 @@ func TestQueryOneToOne(t *testing.T) {
executeTestCase(t, test)
}
}

func TestQueryOneToOneWithMultipleRecords(t *testing.T) {
test := testUtils.QueryTestCase{
Description: "One-to-one relation primary direction, multiple records",
Query: `query {
book {
name
author {
name
}
}
}`,
Docs: map[int][]string{
//books
0: {
// bae-fd541c25-229e-5280-b44b-e5c2af3e374d
`{
"name": "Painted House",
"rating": 4.9
}`,
// "bae-d3bc0f38-a2e1-5a26-9cc9-5b3fdb41c6db"
`{
"name": "Go Guide for Rust developers",
"rating": 5.0
Comment on lines +124 to +132
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why not keep the 4 fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are irrelevant, and including them suggests relevance

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little strict xD. It doesn't hinder understanding with them in, however it also doesn't add anything

Copy link
Contributor Author

@AndrewSisley AndrewSisley Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it does, when tracking down the bug the first thing I did (and this was about 50% of the effort) was remove everything extra so I had a minimal failing case. Specificity matters a lot in tests IMO. Really cuts down the possible what-if cases when looking to see why it is failing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair 👍

}`,
},
//authors
1: {
// "bae-3bfe0092-e31f-5ebe-a3ba-fa18fac448a6"
`{
"name": "John Grisham",
"age": 65,
"verified": true,
"published_id": "bae-fd541c25-229e-5280-b44b-e5c2af3e374d"
}`,
// "bae-756c2bf0-4767-57fd-b12b-393915feae68",
`{
"name": "Andrew Lone",
"age": 30,
"verified": true,
"published_id": "bae-d3bc0f38-a2e1-5a26-9cc9-5b3fdb41c6db"
}`,
},
},
Results: []map[string]any{
{
"name": "Go Guide for Rust developers",
"author": map[string]any{
"name": "Andrew Lone",
},
},
{
"name": "Painted House",
"author": map[string]any{
"name": "John Grisham",
},
},
},
}

executeTestCase(t, test)
}