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 2 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
2 changes: 1 addition & 1 deletion planner/filter/complex.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ 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 {
if v, ok := v.([]any); ok && len(v) == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not ideal to keep this as-is long-term, as it will prevent a possible performance-shortcut once John's fetcher rework gets merged, but for now it is a simple way of excluding the index fetcher filter's lack of support for compound filters..

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are going to do this might as well remove the if block all together. It's never going to be of length 0.

Copy link
Contributor Author

@AndrewSisley AndrewSisley May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Will do. I think it is valid to submit an empty and/or clause, but probably not really worth the extra code for that :)

  • Remove len 0 check

continue
}
if isComplex(v, 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 cocuments read and ready to be yielded by this node.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: documents

Copy link
Contributor Author

@AndrewSisley AndrewSisley May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

:) will fix

  • typo

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