Add filtering ability for all backend API ListXXX requests #537
Conversation
Add tests for list package, and let ExperimentStore use this new API. Update tests for the latter as well.
|
/cc @yebrahim |
|
/assign @IronPan |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vicaire The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| LESS_THAN_EQUALS = 7; | ||
|
|
||
| // Checks if the value is a member of a given array, which should be one of | ||
| // |int_values|, |long_values| or |string_values|. |
There was a problem hiding this comment.
But not timestamp_value? If there is a good reason for this, we should point it out here.
There was a problem hiding this comment.
There's no good reason other than I thought it unlikely we'd want to query specific dates vs a range of dates. If we ever want to do so, we can add it easily.
| // } | ||
| // } | ||
| message Filter { | ||
| // All predicates are AND-ed when this filter is applied. |
There was a problem hiding this comment.
nit: Does this description really belong here? This seems to me like a backend logic contract rather than protocol, no?
There was a problem hiding this comment.
The logic of the filter is part of the contract, so I think having it here is useful for anyone trying to create a Filter proto.
|
|
||
| string key = 2; | ||
| oneof value { | ||
| int32 int_value = 3; |
There was a problem hiding this comment.
I don't think this is needed for a service interface. Clients usually aren't very careful about numeric type accuracy (not to mention our clients are dynamically typed languages anyway).
Is it safe to say that unless this proto is to be used between backend services, it doesn't make sense to use integers anywhere?
There was a problem hiding this comment.
I don't think so. The client can be in any language that supports protos, like Go. In which case types do matter.
|
@yebrahim I can send out another PR with changes if you'd like me to make any. Please let me know. Thanks! |
|
I don't have very strong opinions on the comments here, so I'm fine with these resolutions. |
This PR adds filtering capability to all ListXXX requests in the backend API. As the context for filtering needs to be preserved with that of sorting, I've unified both under the new 'list' package, which attempts to streamline the previous logic for sorting and generating next page tokens. High-level overview of what's in this PR:
Filterprotocol buffer which is used to transmit filtering requests from the frontend to the backend.filterhandles parsing the Filter proto and is able to apply corresponding WHERE clauses when constructing SQL statements.listhandles both sorting and filtering, and is used by all ListXXX requests down the stack. It generates next page tokens to ensure both sorting and filtering criteria carries over to the next page of results.This change is