feat: Inline array filters #3028
Conversation
nasdf
added
feature
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3028 +/- ##
===========================================
+ Coverage 79.38% 79.42% +0.04%
===========================================
Files 333 336 +3
Lines 25837 26072 +235
===========================================
+ Hits 20509 20706 +197
- Misses 3864 3889 +25
- Partials 1464 1477 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| // matching if all of them match. | ||
| func all(condition, data any) (bool, error) { | ||
| switch t := data.(type) { | ||
| case []string: |
There was a problem hiding this comment.
todo: I think it's better to use reflection here to avoid repetitive code:
func all(condition, data any) {
val := reflect.ValueOf(v)
if val.Kind() != reflect.Slice {
return false, client.NewErrUnhandledType("data", data)
}
for i := 0; i < val.Len(); i++ {
m, err := eq(condition, val.Index(i).Interface())
if err != nil {
return false, err
} else if !m {
return false, nil
}
}
}
return true, nil
}Or at least use generic:
func allInArr[T any](slice []T, condition any) (bool, error) {
for _, item := range slice {
m, err := eq(condition, item)
if err != nil {
return false, err
}
if !m {
return false, nil
}
}
return true, nil
}
func all(condition, data any) (bool, error) {
switch t := data.(type) {
case []string:
return allInArr(t, condition)
case []immutable.Option[string]:
return allInArr(t, condition)
case []int64:
return allInArr(t, condition)
// ...This will also improve test coverage.
There was a problem hiding this comment.
reflect is a bit slow and not fully supported in wasm so lets try to avoid it for now. I like the generic idea though.
There was a problem hiding this comment.
100% agree with Fred, we should avoid reflect, especially in a performance sensitive area like this. Extracting the shared logic to a generic func would be good though.
| // matching if any of them match. | ||
| func anyOp(condition, data any) (bool, error) { | ||
| switch t := data.(type) { | ||
| case []string: |
There was a problem hiding this comment.
todo: please use the same approach as with all
internal/connor/eq.go
Outdated
| if arr.HasValue() { | ||
| data = arr.Value() | ||
| } else { | ||
| data = nil |
There was a problem hiding this comment.
suggestion: this could also be turned into generic code
| Fields: gql.InputObjectConfigFieldMap{ | ||
| "_any": &gql.InputObjectFieldConfig{ | ||
| Description: anyOperatorDescription, | ||
| Type: op, | ||
| }, | ||
| "_all": &gql.InputObjectFieldConfig{ | ||
| Description: allOperatorDescription, | ||
| Type: op, | ||
| }, | ||
| "_none": &gql.InputObjectFieldConfig{ | ||
| Description: noneOperatorDescription, | ||
| Type: op, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
suggestion: this can be extracted into a function and used everywhere
There was a problem hiding this comment.
I have mixed feelings about the extraction of the full InputObjectFieldConfig blocks to a shared func (it couples them together, and hides what each InputObject contains, yet the enforcement of consistency is nice).
There was a problem hiding this comment.
My preference is to keep them as is because it makes modifying each individual type easier.
| testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
| ) | ||
|
|
||
| func TestQueryInlineStringArrayWithAllFilter(t *testing.T) { |
There was a problem hiding this comment.
suggestion: we try to stick to the following test naming convention:
<Category>_<Condition|Prerequisite>_<Result|Expectation>
In this case it could be
TestQueryInlineStringArray_WithAllFilter_Succeeds
There was a problem hiding this comment.
nitpick: I did think we agreed to not force that suggestion upon developers though, especially the last block.
There was a problem hiding this comment.
I agree that that last part is optional
| Users(filter: {testScores: {_all: {_lt: 70}}}) { | ||
| name | ||
| } |
There was a problem hiding this comment.
suggestion: would be also great to see a reassurance (in form of another test) that this new op works with compound conditions. For example:
filter: {testScores: {_all: {_and: [{_lt: 70}, {_gte: 0}]}}}
And to do the same with other new ops.
There was a problem hiding this comment.
I've added two compound tests.
| notNullBooleanOpBlock := schemaTypes.NotNullBooleanOperatorBlock() | ||
| notNullStringOpBlock := schemaTypes.NotNullStringOperatorBlock() | ||
| notNullJSONOpBlock := schemaTypes.NotNullJSONOperatorBlock(jsonScalarType) | ||
| notNullBlobOpBlock := schemaTypes.NotNullBlobOperatorBlock(blobScalarType) |
There was a problem hiding this comment.
question: I'm curious - why did you pull the existing stuff out of the return and into temp vars? And then add new func calls within the return?
There was a problem hiding this comment.
It made sense to me organizationally. Also the temp vars are used as inputs to the calls in the return.
| Name: "BooleanListOperatorBlock", | ||
| Description: "These are the set of filter operators available for use when filtering on [Boolean] values.", | ||
| Fields: gql.InputObjectConfigFieldMap{ | ||
| "_any": &gql.InputObjectFieldConfig{ |
There was a problem hiding this comment.
suggestion: Independent of Islam's suggestion, it might be nice to extract the names to consts in the client/request package and then also reference them in the connor package as well as here.
There was a problem hiding this comment.
This is out of scope for this PR imo, but I will create an issue for this.
Relevant issue(s)
Resolves #2857
Description
This PR adds three new filter types for inline array types:
_all,_any, and_none.Tasks
How has this been tested?
Added integration tests
Specify the platform(s) on which this was tested: