feat: field selection for search #377
Conversation
adilansari
commented
Jul 21, 2022
adilansari
commented
- Ability to request certain document fields from search results, by inclusion or exclusion:
- Fixed a minor bug where facet sizes was not being respected in response. Only the max size was being honored
|
do we need true/false explicitly? how about this and for the other scenario what do you think? |
|
please add test exercising this code path. |
query/search/search.go
Outdated
| Facets Facets | ||
| PageSize int | ||
| WrappedF *filter.WrappedFilter | ||
| FieldSelection *read.FieldFactory |
There was a problem hiding this comment.
just have a simple list, no need to have it as a map because I don't see in the future if we need to add any functionality inside the fields object
server/services/v1/search_reader.go
Outdated
| // apply the field selection | ||
| if p.fieldSelection != nil { | ||
| newValue, err := p.fieldSelection.Apply(rawData) | ||
| if ulog.E(err) { |
There was a problem hiding this comment.
What about user input is a list but in code, we can build the fields using read.FieldFactory then we can still use this method but user input is simple.
There was a problem hiding this comment.
I did make the change but doesn't look too intuitive to implicitly assume fields array to inclusion list. It may not appear intuitive if a user is building [a, b] for inclusion list and [a: false, b: false] for exclusion. Even if this array is easy to use, we should rather have include and exclude explicitly
There was a problem hiding this comment.
No, I was not saying it to have [a: false, b: false]. I was saying to just have an array [a, b], similarly for exclude it will be an array.
query/read/fields.go
Outdated
| @@ -37,6 +38,37 @@ func BuildFields(reqFields jsoniter.RawMessage) (*FieldFactory, error) { | |||
| factory.Include = make(map[string]Field) | |||
| factory.Exclude = make(map[string]Field) | |||
|
|
|||
| var v interface{} | |||
There was a problem hiding this comment.
allowing read fields input to be an array, will update user facing documentation for both APIs separately
So should we make an API change now for both read and search to accept, |
So we don't need to make any changes to read because read supports For search, the value is not an expression. The only value for it would be Thinking more about it, I think either approach should be fine. to support it as an array with explicit exclude/include or to have a map. |
query/read/fields.go
Outdated
| // it's something else | ||
| err = api.Errorf(api.Code_INVALID_ARGUMENT, "fields selection can only be array or object") | ||
| } | ||
|
|
There was a problem hiding this comment.
nit, remove the extra line.
query/read/fields.go
Outdated
| } | ||
|
|
||
| var err error | ||
| switch v := v.(type) { |
There was a problem hiding this comment.
What will we have in the open API spec? Object or array?
There was a problem hiding this comment.
if it is confusing, I am fine to stick with having it as an object to avoid any confusion and if needed in clients we can have an array.
|
If we have to separate this from Read api (because read will evolve into aggregations etc.), I'd rather make a change now to |
|
This PR now only has fix for facet sizes, I'll send a separate one for API change |
Codecov Report
@@ Coverage Diff @@
## main #377 +/- ##
==========================================
- Coverage 28.70% 28.68% -0.03%
==========================================
Files 66 66
Lines 6936 6941 +5
==========================================
Hits 1991 1991
- Misses 4718 4723 +5
Partials 227 227
Continue to review full report at Codecov.
|
|
🎉 This PR is included in version 1.0.0-alpha.24 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
