Add GRPC server query RPC endpoints #152
Conversation
| @@ -45,6 +46,23 @@ func (u ValueUnion) MarshalJSON() ([]byte, error) { | |||
| } | |||
| } | |||
|
|
|||
| // ToProto converts a value to a calculation value protobuf message. | |||
| func (u ValueUnion) ToProto() servicepb.CalculationValue { | |||
There was a problem hiding this comment.
is this safe? rename to Must...? Also feels more readable to put panic in default:
There was a problem hiding this comment.
This is safe yeah. Sure can move panic to default.
| OptionalFilterValue value = 3 [(gogoproto.nullable) = false]; | ||
| } | ||
|
|
||
| message OptionalString { |
There was a problem hiding this comment.
nit: move all the Optional types together?
| message OptionalString { | ||
| oneof value { | ||
| bool no_value = 1; | ||
| string data = 2; |
There was a problem hiding this comment.
This is only used in query proto to specify things like fields to filter against, so it's more readable to use strings on the client side.
| message WriteResults {} | ||
|
|
||
| message RawQueryResults { | ||
| repeated string raw = 1; |
There was a problem hiding this comment.
Is this intended for performance reasons? or is string enough for the results?
There was a problem hiding this comment.
No this is to return a list of raw results, so you need to have an array here.
| Type type = 1; | ||
| bool bool_val = 2; | ||
| double number_val = 3; | ||
| string string_val = 4; |
There was a problem hiding this comment.
This is only used in the query filter in a query expression and is probably more readable to use strings. It should have no impact on performance.
| } | ||
|
|
||
| enum SortOrder { | ||
| UNKNOWNSORTORDER = 0; |
There was a problem hiding this comment.
Is this needed? Should it be by default ASCENDING? Or handle that logic in converting functions?
There was a problem hiding this comment.
I generally prefer to have the default slot to be something unknown and make this explicitly optional if needed (i.e., handling in converting functions)
cc @black-adder @cw9 @notbdu
This PR adds gRPC server query RPC endpoints.
Will be addressed in followup PRs: