Add filedb filter logic #3389
Add filedb filter logic #3389
Conversation
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
| os, err := makeSliceOfInterfaces(operand) | ||
| if err != nil { | ||
| return false, fmt.Errorf("operand error: %w", err) | ||
| } | ||
|
|
||
| for _, o := range os { | ||
| if o == val { | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil |
There was a problem hiding this comment.
nits, it would be better to make this implementation a function and return directory.
There was a problem hiding this comment.
I think it's not that different since the logic to return is different between cases 🤔
|
Code coverage for golang is
|
|
Nice! |
pkg/datastore/filedb/filter.go
Outdated
| case int, int8, int16, int32, int64: | ||
| valNum = reflect.ValueOf(v).Int() | ||
| case uint, uint8, uint16, uint32: | ||
| valNum = reflect.ValueOf(v).Int() |
There was a problem hiding this comment.
| valNum = reflect.ValueOf(v).Int() | |
| valNum = int64(reflect.ValueOf(v).Uint()) |
Sorry for the lack of explanation.
if the underlying type is uint, Int() will be panic(https://pkg.go.dev/reflect#Value.Int). So calling Uint() and then casting to int is better.
There was a problem hiding this comment.
oops, thanks for explanation 👍
|
Code coverage for golang is
|
|
It looks good to me 👍 |
| // TODO: Handle nested field name such as SyncState.Status. | ||
| return false, datastore.ErrUnsupported |
There was a problem hiding this comment.
Idea: Or to be simple, we can avoid using nested fields by double storing the SyncState_Status in the Application model.
There was a problem hiding this comment.
I see, I planned to make logic like flatten nested json and do filter logic on the output map. But you're right, we may avoid complexity by unuse nested fields query. Will think about it and address with another PR 🙏
| Match(e interface{}, filters []datastore.ListFilter) (bool, error) | ||
| } | ||
|
|
||
| func filter(col datastore.Collection, e interface{}, filters []datastore.ListFilter) (bool, error) { |
There was a problem hiding this comment.
Return fast when there was no filter to avoid unneeded marshaling.
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Handle nested field name such as SyncState.Status.pipecd/pkg/datastore/filedb/filter.go Lines 56 to 59 in fad8615 This was created by todo plugin since "TODO:" was found in fad8615 when #3389 was merged. cc: @khanhtc1202. |
| // remarshal entity as map[string]interface{} struct. | ||
| raw, err := json.Marshal(e) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| var omap map[string]interface{} | ||
| if err := json.Unmarshal(raw, &omap); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Any way to deal with the entity struct directly instead of remarshal like this?
I feel that we are having many marshal-unmarshal around this part.
There was a problem hiding this comment.
To iterate throw struct and get field name as string, we only have using reflect or remarshal it to map[string]interface{} for now. I tried to do filter before first unmarshal (from object parts to object full data) but it does not work, since in some cases, the fields' values are stored duplicated between object parts, which make it's required to merge (unmarshal all object parts and build the whole object) once, then filter based on the data fulfilled object.
Besides, I read around the JSON marshal logic of the standard lib, and looks like it's called good/fast enough compare with others marshaling packages. So for now, I guess, I would like to make a try this method. If it's too costly this way, I will reconsider using reflect package or such. Wdyt?
There was a problem hiding this comment.
Totally agree about that. Let's give it a try and see how it work. 👍
|
Code coverage for golang is
|
|
Nice. Let's merge and see how it work. /approve |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR adds filter logic which is used in the Find interface. Previously, I planned to make filter logic as part of collection logic and implement it for each model kind. Since the filter logic accepts
Filter.Field(a string) as its input. If we make that filter logic as part of collections, we may have to use reflect to list up struct's fields name as string, it's complicated and not worth it. In this PR, instead of making filter logic as part of collections, it will be addressed in filedb layer, with a more generic interfacemap[string]interface{}as the input entity.We still accept filter logic in collections (if exists) and only use this generic filter logic in case the collection does not implement the
filterableinterface.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: