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

feat: Add support for aggregate filters on inline arrays #622

Merged
merged 5 commits into from
Jul 13, 2022
Merged

feat: Add support for aggregate filters on inline arrays #622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bc12d3
Remove out of date comment
AndrewSisley Jul 11, 2022
eee8aad
Remove unuseful 0 check
AndrewSisley Jul 11, 2022
c13172b
Remove filter type restrictions
AndrewSisley Jul 11, 2022
fac4858
Add support for inline array aggregate filters
AndrewSisley Jul 11, 2022
ce8d4dc
PR Suggestion - remove length variable
AndrewSisley Jul 12, 2022
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
4 changes: 1 addition & 3 deletions connor/connor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ package connor

import (
"fmt"

"github.com/sourcenetwork/defradb/core"
)

// Match is the default method used in Connor to match some data to a
// set of conditions.
func Match(conditions map[FilterKey]interface{}, data core.Doc) (bool, error) {
func Match(conditions map[FilterKey]interface{}, data interface{}) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: you changed it to core.Doc previously and now back to interface{}. We are no longer just comparing core.Doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this now needs to handle inline arrays (noted in commit message)

Copy link
Collaborator

@fredcarle fredcarle Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(noted in commit message)

well not explicitly 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless I fluffed up the commit contents, the message is:

Remove filter type restrictions
This needs to be able to accomodate inline arrays shortly

return eq(conditions, data)
}

Expand Down
15 changes: 9 additions & 6 deletions query/graphql/mapper/mapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,15 @@ func resolveAggregates(
fieldDesc, isField := desc.GetField(target.hostExternalName)
if isField && !fieldDesc.IsObject() {
// If the hostExternalName matches a non-object field
// we can just take it as a field-requestable as only
// objects are targetable-requestables.
// we don't have to search for it and can just construct the
// targeting info here.
hasHost = true
host = &Field{
Index: int(fieldDesc.ID),
Name: target.hostExternalName,
host = &Targetable{
Field: Field{
Index: int(fieldDesc.ID),
Name: target.hostExternalName,
},
Filter: ToFilter(target.filter, mapping),
}
} else {
childObjectIndex := mapping.FirstIndexOfName(target.hostExternalName)
Expand Down Expand Up @@ -828,7 +831,7 @@ func toOrderBy(source *parserTypes.OrderBy, mapping *core.DocumentMapping) *Orde

// RunFilter runs the given filter expression
// using the document, and evaluates.
func RunFilter(doc core.Doc, filter *Filter) (bool, error) {
func RunFilter(doc interface{}, filter *Filter) (bool, error) {
if filter == nil {
return true, nil
}
Expand Down
4 changes: 4 additions & 0 deletions query/graphql/mapper/targetable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,3 +151,7 @@ func (t *Targetable) cloneTo(index int) *Targetable {
OrderBy: t.OrderBy,
}
}

func (t *Targetable) AsTargetable() (*Targetable, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: in the file requestable.go should probably add these checks if we want Targetable and AggregateTarget to satisfy Requestable (which they currently do):

	_ Requestable = (*Targetable)(nil)
	_ Requestable = (*AggregateTarget)(nil)

question: I can see the benefit in turning the embedded type Field, in Targetable struct into a named type because all Requestable methods that Field implements are dropped into Targetable making it satisfy Requestable interface implicitly?. If in future a dev removes Targetable.AsTargetable() method (which returns true) then the method Targetable.Field.AsTargetable() will automatically take over now returning false. If Field is named then we would force the user to be explicit about which method to use? Also if we do want Targetable to satisfy Requestable then perhaps for readability it might be better to have those methods explicitly defined and not inherited by the embedded type?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

in the file requestable.go should probably add these checks if we want Targetable and AggregateTarget to satisfy Requestable (which they currently do)

Good shout - will check/add (might already do)

  • compiler stuff

dropped into Targetable making it satisfy Requestable interface implicitly...

This is deliberate and desirable IMO

we would force the user to be explicit about which method to use

The user should not have a choice here, or have to worry about it.

perhaps for readability it might be better to have those methods explicitly defined and not inherited by the embedded type?

Maybe - I'm 50-50 here, there are better ways of handling this in other languages. Inheritance in data-structures is something I like quite a lot, but there is no good way of doing this in Go atm (maybe when we get generics, but even then I'm not sure due to Golangs embedding mechanic - I never looked at whether they allow generic constraints to account for embedded types). I don't think this is worth really paying much attention to atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the file requestable.go should probably add these checks if we want Targetable and AggregateTarget to satisfy Requestable (which they currently do)

Rejected on further look. Targetable and AggregateTarget are not Requestable and should not implement that interface (other than accidentally), that they implement some of their functions is coincidence and not a requirement (this coincidence is used here, but not required).

return t, true
}
58 changes: 49 additions & 9 deletions query/graphql/planner/count.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,54 @@ func (n *countNode) Next() (bool, error) {
switch v.Kind() {
// v.Len will panic if v is not one of these types, we don't want it to panic
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I feel like this entire for loop can be more simpler, consider the following:

	for _, source := range n.aggregateMapping {
		property := n.currentValue.Fields[source.Index]

		v := reflect.ValueOf(property)
		// isValueOfPropertyValid is true if v is one of the reflect types that can call v.Len() without panic.
		isValueOfPropertyValid := false
		switch v.Kind() {
		case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String:
			isValueOfPropertyValid = true
		}

		if !isValueOfPropertyValid {
			continue
		}

		// v.Len() will panic if isValueOfPropertyValid is false.
		if source.Filter == nil {
			count = count + v.Len()
			continue
		}

		for i := 0; i < v.Len(); i++ {
			var passed bool
			var err error

			switch array := property.(type) {
			case []core.Doc:
				passed, err = mapper.RunFilter(array[i], source.Filter)
			case []bool:
				passed, err = mapper.RunFilter(array[i], source.Filter)
			case []int64:
				passed, err = mapper.RunFilter(array[i], source.Filter)
			case []float64:
				passed, err = mapper.RunFilter(array[i], source.Filter)
			case []string:
				passed, err = mapper.RunFilter(array[i], source.Filter)
			}

			if err != nil {
				return false, err
			}
			if passed {
				count += 1
			}
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion contains less code duplication, but I'm not sure it is simpler, and I'm really not a fan of isValueOfPropertyValid :)

Breaking up the switch cases breaks up the code-flow and makes reading it harder IMO. You also dont want to move switch array := property.(type) { inside the array loop, as that is a waste of resources.

This can be shrunk when we have generics, but until then I'm happy as-is

Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: you don't have to do a type switch. You can simply get the value of the property and pass it to RunFilter.

for _, source := range n.aggregateMapping {
		// track if property is a Slice or Array
		isIterable := false

		property := n.currentValue.Fields[source.Index]
		v := reflect.ValueOf(property)
		// isValueOfPropertyValid is true if v is one of the reflect types that can call v.Len() without panic.
		switch v.Kind() {
		case reflect.Array, reflect.Slice:
			isIterable = true
		case reflect.Chan, reflect.Map, reflect.String:
		default:
			continue
		}

		// v.Len() will panic if isValueOfPropertyValid is false.
		if source.Filter == nil {
			count = count + v.Len()
			continue
		}
        
		if isIterable {
			for i := 0; i < v.Len(); i++ {
				passed, err := mapper.RunFilter(v.Index(i).Interface(), source.Filter)
				if err != nil {
					return false, err
				}
				if passed {
					count += 1
				}
			}
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) I tried something very similar briefly (is a cleaner way that avoids isIterable and just appends the length) - but I missed the Interface() call and shrugged it off pretty quick as not working.

Again, this is trivial to shrink with generics (avoiding the runtime cost of reflect/casting/etc) - so I really don't think it matters at all. Do you have a strong preference to using something similar to the above in the short-term, or are you happy to wait for generics?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have too strong of a preference, but since you are already changing the function it would be nice to clean it up until we get generics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference in cost between changing it now (I have no plans to do so unless you guys really want), and changing it in a month or two is minimal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your initial version works and is readable. I don't like the repetition but if you think it will be cleaned up with generics in the near future, then just go with whatever you prefer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket added in case I forget/get-lazy #633

case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String:
length := v.Len()
// For now, we only support count filters internally to support averages
// so this is fine here now, but may need to be moved later once external
// count filter support is added.
if count > 0 && source.Filter != nil {
docArray, isDocArray := property.([]core.Doc)
if isDocArray {
for _, doc := range docArray {
if source.Filter != nil {
switch array := property.(type) {
case []core.Doc:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking forward to getting generics lol

for _, doc := range array {
passed, err := mapper.RunFilter(doc, source.Filter)
if err != nil {
return false, err
}
if passed {
count += 1
}
}

case []bool:
for _, doc := range array {
passed, err := mapper.RunFilter(doc, source.Filter)
if err != nil {
return false, err
}
if passed {
count += 1
}
}

case []int64:
for _, doc := range array {
passed, err := mapper.RunFilter(doc, source.Filter)
if err != nil {
return false, err
}
if passed {
count += 1
}
}

case []float64:
for _, doc := range array {
passed, err := mapper.RunFilter(doc, source.Filter)
if err != nil {
return false, err
}
if passed {
count += 1
}
}

case []string:
for _, doc := range array {
passed, err := mapper.RunFilter(doc, source.Filter)
if err != nil {
return false, err
Expand All @@ -116,7 +156,7 @@ func (n *countNode) Next() (bool, error) {
}
}
} else {
count = count + length
count = count + v.Len()
}
}
}
Expand Down
14 changes: 14 additions & 0 deletions query/graphql/planner/sum.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,24 @@ func (n *sumNode) Next() (bool, error) {
}
case []int64:
for _, childItem := range childCollection {
passed, err := mapper.RunFilter(childItem, source.Filter)
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: perhaps can be simplified too similar to the countNode.Next() suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my response to that comment. I'll apply anything that comes out of that convo here though if suitable

if err != nil {
return false, err
}
if !passed {
continue
}
sum += float64(childItem)
}
case []float64:
for _, childItem := range childCollection {
passed, err := mapper.RunFilter(childItem, source.Filter)
if err != nil {
return false, err
}
if !passed {
continue
}
sum += childItem
}
}
Expand Down
21 changes: 17 additions & 4 deletions query/graphql/schema/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,15 +291,28 @@ func (g *Generator) createExpandedFieldAggregate(
) {
for _, aggregateTarget := range f.Args {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Maybe the for loop can be one less nested-level with:

	for _, aggregateTarget := range f.Args {
		target := aggregateTarget.Name()
		var filterTypeName string
		filterType := obj.Fields()[target].Type
		if target == parserTypes.GroupFieldName {
			filterTypeName = obj.Name() + "FilterArg"
		} else if list, isList := filterType.(*gql.List); isList && gql.IsLeafType(list.OfType) {
			// If it is a list of leaf types - the filter is just the set of OperatorBlocks
			// that are supported by this type - there can be no field selections.
			if notNull, isNotNull := list.OfType.(*gql.NonNull); isNotNull {
				// GQL does not support '!' in type names, and so we have to manipulate the
				// underlying name like this if it is a nullable type.
				filterTypeName = fmt.Sprintf("NotNull%sOperatorBlock", notNull.OfType.Name())
			} else {
				filterTypeName = genTypeName(list.OfType, "OperatorBlock")
			}
		} else {
			filterTypeName = filterType.Name() + "FilterArg"
		}

		expandedField := &gql.InputObjectFieldConfig{
			Type: g.manager.schema.TypeMap()[filterTypeName],
		}
		aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)
	}

Copy link
Contributor Author

@AndrewSisley AndrewSisley Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That would expand the scope of filterType, and you'd have to worry about whether obj.Fields()[target].Typemight panic if target == parserTypes.GroupFieldName - not worth it IMO

target := aggregateTarget.Name()
var targetType string
var filterTypeName string
if target == parserTypes.GroupFieldName {
targetType = obj.Name()
filterTypeName = obj.Name() + "FilterArg"
} else {
targetType = obj.Fields()[target].Type.Name()
filterType := obj.Fields()[target].Type
if list, isList := filterType.(*gql.List); isList && gql.IsLeafType(list.OfType) {
// If it is a list of leaf types - the filter is just the set of OperatorBlocks
// that are supported by this type - there can be no field selections.
if notNull, isNotNull := list.OfType.(*gql.NonNull); isNotNull {
// GQL does not support '!' in type names, and so we have to manipulate the
// underlying name like this if it is a nullable type.
filterTypeName = fmt.Sprintf("NotNull%sOperatorBlock", notNull.OfType.Name())
} else {
filterTypeName = genTypeName(list.OfType, "OperatorBlock")
}
} else {
filterTypeName = filterType.Name() + "FilterArg"
}
}

expandedField := &gql.InputObjectFieldConfig{
Type: g.manager.schema.TypeMap()[targetType+"FilterArg"],
Type: g.manager.schema.TypeMap()[filterTypeName],
}
aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField)
aggregateTarget.Type.(*gql.InputObject).AddFieldConfig(parserTypes.FilterClause, expandedField)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggest out of scope, and there are a few of these dotted about in here that can be cleaned up in one go.

}
Expand Down
4 changes: 4 additions & 0 deletions query/graphql/schema/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,15 @@ func defaultTypes() []gql.Type {

// Filter scalar blocks
booleanOperatorBlock,
notNullBooleanOperatorBlock,
dateTimeOperatorBlock,
floatOperatorBlock,
notNullFloatOperatorBlock,
idOperatorBlock,
intOperatorBlock,
notNullIntOperatorBlock,
stringOperatorBlock,
notNullstringOperatorBlock,

schemaTypes.CommitLinkObject,
schemaTypes.CommitObject,
Expand Down
100 changes: 100 additions & 0 deletions query/graphql/schema/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,25 @@ var booleanOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
},
})

// notNullBooleanOperatorBlock filter block for boolean! types.
var notNullBooleanOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "NotNullBooleanOperatorBlock",
Fields: gql.InputObjectConfigFieldMap{
"_eq": &gql.InputObjectFieldConfig{
Type: gql.Boolean,
},
"_ne": &gql.InputObjectFieldConfig{
Type: gql.Boolean,
},
"_in": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Boolean)),
},
"_nin": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Boolean)),
},
},
})

// dateTimeOperatorBlock filter block for DateTime types.
var dateTimeOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "DateTimeOperatorBlock",
Expand Down Expand Up @@ -111,6 +130,37 @@ var floatOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
},
})

// notNullFloatOperatorBlock filter block for Float! types.
var notNullFloatOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "NotNullFloatOperatorBlock",
Fields: gql.InputObjectConfigFieldMap{
"_eq": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_ne": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_gt": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_ge": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_lt": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_le": &gql.InputObjectFieldConfig{
Type: gql.Float,
},
"_in": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Float)),
},
"_nin": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Float)),
},
},
})

// intOperatorBlock filter block for Int types.
var intOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "IntOperatorBlock",
Expand Down Expand Up @@ -142,6 +192,37 @@ var intOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
},
})

// notNullIntOperatorBlock filter block for Int! types.
var notNullIntOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "NotNullIntOperatorBlock",
Fields: gql.InputObjectConfigFieldMap{
"_eq": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_ne": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_gt": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_ge": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_lt": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_le": &gql.InputObjectFieldConfig{
Type: gql.Int,
},
"_in": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Int)),
},
"_nin": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.Int)),
},
},
})

// stringOperatorBlock filter block for string types.
var stringOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "StringOperatorBlock",
Expand All @@ -164,6 +245,25 @@ var stringOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
},
})

// notNullstringOperatorBlock filter block for string! types.
var notNullstringOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "NotNullStringOperatorBlock",
Fields: gql.InputObjectConfigFieldMap{
"_eq": &gql.InputObjectFieldConfig{
Type: gql.String,
},
"_ne": &gql.InputObjectFieldConfig{
Type: gql.String,
},
"_in": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.String)),
},
"_nin": &gql.InputObjectFieldConfig{
Type: gql.NewList(gql.NewNonNull(gql.String)),
},
},
})

// idOperatorBlock filter block for ID types.
var idOperatorBlock = gql.NewInputObject(gql.InputObjectConfig{
Name: "IDOperatorBlock",
Expand Down
Loading