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

fix: Panic with different composite-indexed child objects #2947

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
12 changes: 6 additions & 6 deletions internal/planner/multi.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2022 Democratized Data Foundation
// Copyright 2024 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
Expand Down Expand Up @@ -214,22 +214,22 @@ func (s *selectNode) addSubPlan(fieldIndex int, newPlan planNode) error {
if err := s.planner.walkAndReplacePlan(s.source, origScan, multiscan); err != nil {
return err
}
// create multinode
multinode := &parallelNode{
// create parallelNode
parallelNode := &parallelNode{
p: s.planner,
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: I forgot to add to my earlier review, thanks for renaming this.

multiscan: multiscan,
docMapper: docMapper{s.source.DocumentMap()},
}
multinode.addChild(-1, s.source)
parallelNode.addChild(-1, s.source)
multiscan.addReader()
// replace our new node internal scanNode with our new multiscanner
if err := s.planner.walkAndReplacePlan(newPlan, origScan, multiscan); err != nil {
return err
}
// add our newly updated plan to the multinode
multinode.addChild(fieldIndex, newPlan)
parallelNode.addChild(fieldIndex, newPlan)
multiscan.addReader()
s.source = multinode
s.source = parallelNode

// we already have an existing parallelNode as our source
case *parallelNode:
Expand Down
57 changes: 39 additions & 18 deletions internal/planner/scan.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2022 Democratized Data Foundation
// Copyright 2024 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
Expand Down Expand Up @@ -325,35 +325,53 @@ func (p *Planner) Scan(
type multiScanNode struct {
scanNode *scanNode
numReaders int
numCalls int

lastBool bool
lastErr error
nextCount int
initCount int
startCount int
closeCount int

nextResult bool
nextErr error
initErr error
startErr error
islamaliev marked this conversation as resolved.
Show resolved Hide resolved
closeErr error
}

func (n *multiScanNode) Init() error {
return n.scanNode.Init()
n.countAndCall(&n.initCount, func() {
n.initErr = n.scanNode.Init()
islamaliev marked this conversation as resolved.
Show resolved Hide resolved
})
return n.initErr
}

func (n *multiScanNode) Start() error {
return n.scanNode.Start()
n.countAndCall(&n.startCount, func() {
n.startErr = n.scanNode.Start()
})
return n.startErr
}

// Next only calls Next() on the underlying
// scanNode every numReaders.
func (n *multiScanNode) Next() (bool, error) {
if n.numCalls == 0 {
n.lastBool, n.lastErr = n.scanNode.Next()
func (n *multiScanNode) countAndCall(count *int, f func()) {
if *count == 0 {
f()
}
n.numCalls++
*count++

// if the number of calls equals the numbers of readers
// reset the counter, so our next call actually executes the Next()
if n.numCalls == n.numReaders {
n.numCalls = 0
// reset the counter, so our next call actually executes the function
if *count == n.numReaders {
*count = 0
}
}

// Next only calls Next() on the underlying
// scanNode every numReaders.
func (n *multiScanNode) Next() (bool, error) {
n.countAndCall(&n.nextCount, func() {
n.nextResult, n.nextErr = n.scanNode.Next()
})

return n.lastBool, n.lastErr
return n.nextResult, n.nextErr
}

func (n *multiScanNode) Value() core.Doc {
Expand All @@ -373,7 +391,10 @@ func (n *multiScanNode) Kind() string {
}

func (n *multiScanNode) Close() error {
return n.scanNode.Close()
n.countAndCall(&n.closeCount, func() {
n.closeErr = n.scanNode.Close()
})
return n.closeErr
}

func (n *multiScanNode) DocumentMap() *core.DocumentMapping {
Expand Down
116 changes: 116 additions & 0 deletions tests/integration/index/query_with_composite_inxed_on_relation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
// Copyright 2024 Democratized Data Foundation
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package index

import (
"testing"

testUtils "github.com/sourcenetwork/defradb/tests/integration"
)

// This test was added during https://github.com/sourcenetwork/defradb/issues/2924
// The issue was that [multiScanNode] that keeps track of how many calls to [Next()] should
// be made, would call [Init()] and [Start()] every time without tracking, which would result
// in child nodes being initialized and started multiple times, which in turn created
// index iterators without closing them.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Thanks for taking the time to link this to the ticket, it highlights the importance of the test very nicely.

func TestQueryWithCompositeIndexOnManyToOne_WithMultipleIndexedChildNodes_ShouldSucceed(t *testing.T) {
test := testUtils.TestCase{
Actions: []any{
testUtils.SchemaUpdate{
Schema: `
type User {
name: String
devices: [Device]
}

type Device @index(fields: ["owner_id", "manufacturer_id"]) {
model: String
owner: User
manufacturer: Manufacturer
}

type Manufacturer {
name: String
devices: [Device]
}
`,
},
testUtils.CreateDoc{
DocMap: map[string]any{
"name": "John",
},
},
testUtils.CreateDoc{
CollectionID: 2,
DocMap: map[string]any{
"name": "Apple",
},
},
testUtils.CreateDoc{
CollectionID: 2,
DocMap: map[string]any{
"name": "Sony",
},
},
testUtils.CreateDoc{
CollectionID: 1,
DocMap: map[string]any{
"model": "MacBook Pro",
"owner": testUtils.NewDocIndex(0, 0),
"manufacturer": testUtils.NewDocIndex(2, 0),
},
},
testUtils.CreateDoc{
CollectionID: 1,
DocMap: map[string]any{
"model": "PlayStation 5",
"owner": testUtils.NewDocIndex(0, 0),
"manufacturer": testUtils.NewDocIndex(2, 1),
},
},
testUtils.Request{
Request: `query {
User {
devices {
model
owner {
name
}
manufacturer {
name
}
}
}
}`,
Results: map[string]any{
"User": []map[string]any{
{
"devices": []map[string]any{
{
"model": "MacBook Pro",
"owner": map[string]any{"name": "John"},
"manufacturer": map[string]any{"name": "Apple"},
},
{
"model": "PlayStation 5",
"owner": map[string]any{"name": "John"},
"manufacturer": map[string]any{"name": "Sony"},
},
},
},
},
},
},
},
}

testUtils.ExecuteTestCase(t, test)
}
Loading