feat: Add support for aggregate filters on inline arrays #622
Conversation
AndrewSisley
added
feature
Codecov Report
@@ Coverage Diff @@
## develop #622 +/- ##
===========================================
- Coverage 56.87% 56.85% -0.02%
===========================================
Files 122 122
Lines 14598 14661 +63
===========================================
+ Hits 8302 8336 +34
- Misses 5586 5608 +22
- Partials 710 717 +7
|
|
|
||
| if source.Filter != nil { | ||
| switch array := property.(type) { | ||
| case []core.Doc: |
There was a problem hiding this comment.
Looking forward to getting generics lol
| @@ -158,3 +158,7 @@ func (t *Targetable) cloneTo(index int) *Targetable { | |||
| OrderBy: t.OrderBy, | |||
| } | |||
| } | |||
|
|
|||
| func (t *Targetable) AsTargetable() (*Targetable, bool) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
query/graphql/planner/count.go
Outdated
| @@ -99,13 +99,55 @@ func (n *countNode) Next() (bool, error) { | |||
| // v.Len will panic if v is not one of these types, we don't want it to panic | |||
| case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String: | |||
| length := v.Len() | |||
There was a problem hiding this comment.
suggestion(non-blocking): Since you are already changing this function, can you just call v.Len() on line 161 and remove length?
There was a problem hiding this comment.
True, I can move this now
- move len
| @@ -99,13 +99,55 @@ func (n *countNode) Next() (bool, error) { | |||
| // v.Len will panic if v is not one of these types, we don't want it to panic | |||
There was a problem hiding this comment.
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
}
}
}
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
}
}
}
}There was a problem hiding this comment.
:) 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ticket added in case I forget/get-lazy #633
AndrewSisley
force-pushed
the
sisley/feat/I392-agg-inline-array-filter
branch
from
c61e322 to
12c2c45
Compare
| @@ -223,10 +223,24 @@ func (n *sumNode) Next() (bool, error) { | |||
| } | |||
| case []int64: | |||
| for _, childItem := range childCollection { | |||
| passed, err := mapper.RunFilter(childItem, source.Filter) | |||
There was a problem hiding this comment.
suggestion: perhaps can be simplified too similar to the countNode.Next() suggestion?
There was a problem hiding this comment.
see my response to that comment. I'll apply anything that comes out of that convo here though if suitable
| @@ -291,15 +291,28 @@ func (g *Generator) createExpandedFieldAggregate( | |||
| ) { | |||
| for _, aggregateTarget := range f.Args { | |||
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
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
| } | ||
|
|
||
| expandedField := &gql.InputObjectFieldConfig{ | ||
| Type: g.manager.schema.TypeMap()[targetType+"FilterArg"], | ||
| Type: g.manager.schema.TypeMap()[filterTypeName], | ||
| } | ||
| aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField) |
There was a problem hiding this comment.
| aggregateTarget.Type.(*gql.InputObject).AddFieldConfig("filter", expandedField) | |
| aggregateTarget.Type.(*gql.InputObject).AddFieldConfig(parserTypes.FilterClause, expandedField) | |
There was a problem hiding this comment.
Suggest out of scope, and there are a few of these dotted about in here that can be cleaned up in one go.
| @@ -0,0 +1,71 @@ | |||
| // Copyright 2022 Democratized Data Foundation | |||
| // | |||
There was a problem hiding this comment.
praise: Thanks for taking the time to add these :)
| testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
| ) | ||
|
|
||
| func TestQueryInlineIntegerArrayWithsWithSumWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: typo ? a bit too many withs in WithsWithSumWith haha.
There was a problem hiding this comment.
lol my bad :)
- test name copy-paste typo
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestQueryInlineFloatArrayWithsWithSumWithFilter(t *testing.T) { |
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | ||
| }`)}, |
There was a problem hiding this comment.
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`)}, | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`}, |
There was a problem hiding this comment.
Resolved for all
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | ||
| }`)}, |
There was a problem hiding this comment.
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | |
| }`)}, | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | |
| }`}, |
| testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
| ) | ||
|
|
||
| func TestQueryInlineBoolArrayWithsWithCountWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestQueryInlineIntegerArrayWithsWithCountWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestQueryInlineFloatArrayWithsWithCountWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestQueryInlineStringArrayWithsWithCountWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| Docs: map[int][]string{ | ||
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "PreferredStrings": ["", "the previous", "the first", "empty string"] | ||
| }`)}, | ||
| }, |
There was a problem hiding this comment.
| Docs: map[int][]string{ | |
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "PreferredStrings": ["", "the previous", "the first", "empty string"] | |
| }`)}, | |
| }, | |
| Docs: map[int][]string{ | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "PreferredStrings": ["", "the previous", "the first", "empty string"] | |
| }`}, | |
| }, |
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | ||
| }`)}, |
There was a problem hiding this comment.
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | |
| }`)}, | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.1425, 0.00000000001, 10] | |
| }`}, |
| Docs: map[int][]string{ | ||
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | ||
| }`)}, |
There was a problem hiding this comment.
| Docs: map[int][]string{ | |
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`)}, | |
| Docs: map[int][]string{ | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`}, |
| Docs: map[int][]string{ | ||
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "LikedIndexes": [true, true, false, true] | ||
| }`)}, |
There was a problem hiding this comment.
| Docs: map[int][]string{ | |
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "LikedIndexes": [true, true, false, true] | |
| }`)}, | |
| Docs: map[int][]string{ | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "LikedIndexes": [true, true, false, true] | |
| }`}, |
| testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
| ) | ||
|
|
||
| func TestQueryInlineIntegerArrayWithsWithAverageWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| executeTestCase(t, test) | ||
| } | ||
|
|
||
| func TestQueryInlineFloatArrayWithsWithAverageWithFilter(t *testing.T) { |
There was a problem hiding this comment.
question: same as other comment
| Docs: map[int][]string{ | ||
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteFloats": [3.4, 3.6, 10] | ||
| }`)}, | ||
| }, |
There was a problem hiding this comment.
| Docs: map[int][]string{ | |
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.4, 3.6, 10] | |
| }`)}, | |
| }, | |
| Docs: map[int][]string{ | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteFloats": [3.4, 3.6, 10] | |
| }`}, | |
| }, |
| Docs: map[int][]string{ | ||
| 0: { | ||
| (`{ | ||
| "Name": "Shahzad", | ||
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | ||
| }`)}, | ||
| }, |
There was a problem hiding this comment.
| Docs: map[int][]string{ | |
| 0: { | |
| (`{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`)}, | |
| }, | |
| Docs: map[int][]string{ | |
| 0: { | |
| `{ | |
| "Name": "Shahzad", | |
| "FavouriteIntegers": [-1, 2, -1, 1, 0] | |
| }`}, | |
| }, |
| ) | ||
|
|
||
| // 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) { |
There was a problem hiding this comment.
question: you changed it to core.Doc previously and now back to interface{}. We are no longer just comparing core.Doc?
There was a problem hiding this comment.
Nope, this now needs to handle inline arrays (noted in commit message)
There was a problem hiding this comment.
(noted in commit message)
well not explicitly 😉
There was a problem hiding this comment.
Unless I fluffed up the commit contents, the message is:
Remove filter type restrictions
This needs to be able to accomodate inline arrays shortly
| @@ -99,13 +99,55 @@ func (n *countNode) Next() (bool, error) { | |||
| // v.Len will panic if v is not one of these types, we don't want it to panic | |||
There was a problem hiding this comment.
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
}
}
}
}
This needs to be able to accomodate inline arrays shortly
AndrewSisley
force-pushed
the
sisley/feat/I392-agg-inline-array-filter
branch
from
ad2394b to
ce8d4dc
Compare
…rk#622) * Remove out of date comment * Remove unuseful 0 check * Remove filter type restrictions This needs to be able to accomodate inline arrays shortly * Add support for inline array aggregate filters
Relevant issue(s)
Resolves #392
Description
Add support for aggregate filters on inline arrays.
Does not add and/or support - this has been broken out to #621 as it will only expand on the functionality added in this PR - it doesn't modify the syntax, and keeping it out keeps things a little simpler.