chore: refactor serialization in query service #4234
Conversation
| case float64: | ||
| v.SetBool(n != 0) | ||
| case int64: | ||
| v.SetBool(n != 0) | ||
| case bool: | ||
| v.SetBool(n) | ||
| case string: | ||
| v.SetBool(n == "true") |
There was a problem hiding this comment.
This reminds me of javascript bool 😂
There was a problem hiding this comment.
Copilot reviewed 6 out of 10 changed files in this pull request and generated 3 comments.
Files not reviewed (4)
- frontend/console/src/protos/xyz/block/ftl/query/v1/query_pb.ts: Evaluated as low risk
- go-runtime/ftl/option.go: Evaluated as low risk
- python-runtime/ftl/src/ftl/protos/xyz/block/ftl/query/v1/query_pb2.py: Evaluated as low risk
- python-runtime/ftl/src/ftl/protos/xyz/block/ftl/query/v1/query_pb2.pyi: Evaluated as low risk
Comments suppressed due to low confidence (4)
backend/runner/query/service.go:342
- [nitpick] The parseTimeString function is called for every string parameter, which might not always be a time string. This could lead to unexpected behavior. Consider adding a more robust check or handling for time strings.
if str, ok := param.(string); ok {
backend/protos/xyz/block/ftl/query/v1/query.proto:40
- Ensure that there are test cases covering the parsing of the new 'parameters_json' field to verify that it correctly handles various data types and formats.
string parameters_json = 3; // JSON array of parameter values in order
backend/protos/xyz/block/ftl/query/v1/query.proto:61
- Ensure that there are test cases covering the handling of the new 'json_rows' field to verify that it correctly maps column names to values and handles various data types.
string json_rows = 1; // JSON object mapping column names to values
backend/runner/query/client.go:60
- Ensure that the
paramsvariable is correctly marshaled to JSON and that the resulting JSON string is not empty.
paramsJSON, err := encoding.Marshal(params)
| for i, col := range resultColumns { | ||
| structFields[i] = reflect.StructField{ | ||
| Name: col.TypeName, | ||
| Type: reflect.TypeFor[any](), |
There was a problem hiding this comment.
The function reflect.TypeForany is not valid. It should be reflect.TypeOf(new(any)).Elem().
| Type: reflect.TypeFor[any](), | |
| Type: reflect.TypeOf(new(any)).Elem(), |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -443,11 +478,16 @@ func getDriverName(engine string) string { | |||
| } | |||
|
|
|||
| func handleNoRows(stream *connect.ServerStream[querypb.ExecuteQueryResponse]) error { | |||
| err := stream.Send(&querypb.ExecuteQueryResponse{ | |||
| emptyJSON, err := encoding.Marshal(make(map[string]any)) | |||
There was a problem hiding this comment.
[nitpick] Using encoding.Marshal to create an empty JSON object is unnecessary. Consider using a simpler approach like '{}'.
| emptyJSON, err := encoding.Marshal(make(map[string]any)) | |
| emptyJSON := "{}" |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| for stream.Receive() { | ||
| resp := stream.Msg() | ||
| switch r := resp.Result.(type) { | ||
| case *querypb.ExecuteQueryResponse_ExecResult: | ||
| return nil, nil | ||
| case *querypb.ExecuteQueryResponse_RowResults: | ||
| if len(r.RowResults.Rows) == 0 { | ||
| if r.RowResults.JsonRows == "" { |
There was a problem hiding this comment.
Empty JSON strings should be treated as empty result sets. Consider handling empty JSON strings properly.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
No description provided.