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

Jump to bottom

fix: Handle compound filters on related indexed fields #2575

Merged
merged 4 commits into from
May 2, 2024
Merged

fix: Handle compound filters on related indexed fields #2575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44798fe
Add tests for compound relational index filters
AndrewSisley May 1, 2024
2d6f477
Handle complex filters in compound filters on indexes
AndrewSisley May 1, 2024
164c26a
PR FIXUP - Remove len 0 check from IsComplex
AndrewSisley May 2, 2024
a6db144
PR FIXUP - Fix typo
AndrewSisley May 2, 2024
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
3 changes: 0 additions & 3 deletions planner/filter/complex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ func isComplex(conditions any, seekRelation bool) bool {
if op, ok := k.(*mapper.Operator); ok {
switch op.Operation {
case request.FilterOpOr, request.FilterOpAnd, request.FilterOpNot:
if v, ok := v.([]any); ok && len(v) == 1 {
continue
}
if isComplex(v, true) {
return true
}
Expand Down
2 changes: 1 addition & 1 deletion planner/filter/complex_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func TestIsComplex(t *testing.T) {
inputFilter: r("_or",
m("published", m("rating", m("_gt", 4.0))),
),
isComplex: false,
isComplex: true,
},
{
name: "relation inside _or",
Expand Down
2 changes: 1 addition & 1 deletion planner/planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ func (p *Planner) tryOptimizeJoinDirection(node *invertibleTypeJoin, parentPlan
desc := slct.collection.Description()
for subFieldName, subFieldInd := range filteredSubFields {
indexes := desc.GetIndexesOnField(subFieldName)
if len(indexes) > 0 {
if len(indexes) > 0 && !filter.IsComplex(parentPlan.selectNode.filter) {
subInd := node.documentMapping.FirstIndexOfName(node.subTypeName)
relatedField := mapper.Field{Name: node.subTypeName, Index: subInd}
fieldFilter := filter.UnwrapRelation(filter.CopyField(
Expand Down
33 changes: 30 additions & 3 deletions planner/type_join.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,6 @@ func (dir *joinDirection) invert() {
}

type invertibleTypeJoin struct {
documentIterator
docMapper

root planNode
Expand All @@ -487,6 +486,9 @@ type invertibleTypeJoin struct {
secondaryFieldIndex immutable.Option[int]
secondaryFetchLimit uint

// docsToYield contains documents read and ready to be yielded by this node.
docsToYield []core.Doc

dir joinDirection
}

Expand Down Expand Up @@ -556,6 +558,17 @@ func (join *invertibleTypeJoin) processSecondResult(secondDocs []core.Doc) (any,
}

func (join *invertibleTypeJoin) Next() (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

thought: This function is really difficult to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and IMO the variable names don't really pair up well either, and they actually represent different things in different situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2577

if len(join.docsToYield) > 0 {
// If there is one or more documents in the queue, drop the first one -
// it will have been yielded by the last `Next()` call.
join.docsToYield = join.docsToYield[1:]
if len(join.docsToYield) > 0 {
// If there are still documents in the queue, return true yielding the next
// one in the queue.
return true, nil
}
}

hasFirstValue, err := join.dir.firstNode.Next()

if err != nil || !hasFirstValue {
Expand All @@ -577,7 +590,14 @@ func (join *invertibleTypeJoin) Next() (bool, error) {
return false, err
}
if join.dir.secondNode == join.root {
join.root.Value().Fields[join.subSelect.Index] = join.subType.Value()
if len(secondDocs) == 0 {
return false, nil
}
for i := range secondDocs {
secondDocs[i].Fields[join.subSelect.Index] = join.subType.Value()
}
join.docsToYield = append(join.docsToYield, secondDocs...)
return true, nil
} else {
secondResult, secondIDResult := join.processSecondResult(secondDocs)
join.dir.firstNode.Value().Fields[join.subSelect.Index] = secondResult
Expand All @@ -596,11 +616,18 @@ func (join *invertibleTypeJoin) Next() (bool, error) {
}
}

join.currentValue = join.root.Value()
join.docsToYield = append(join.docsToYield, join.root.Value())

return true, nil
}

func (join *invertibleTypeJoin) Value() core.Doc {
if len(join.docsToYield) == 0 {
return core.Doc{}
}
return join.docsToYield[0]
}

func (join *invertibleTypeJoin) invertJoinDirectionWithIndex(
fieldFilter *mapper.Filter,
index client.IndexDescription,
Expand Down
Loading
Loading