gemini-code-assist[bot] commented on code in PR #39389: URL: https://github.com/apache/beam/pull/39389#discussion_r3615069524
########## sdks/go/pkg/beam/io/bigqueryio/coder.go: ########## @@ -0,0 +1,41 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package bigqueryio + +import ( + "encoding/json" + "fmt" + + "cloud.google.com/go/bigquery" +) + +func encodeQueryParameters(params []bigquery.QueryParameter) ([]byte, error) { + out, err := json.Marshal(params) + if err != nil { + return nil, fmt.Errorf("error encoding JSON: %w", err) + } + + return out, nil +} + +func decodeQueryParameters(bytes []byte) ([]bigquery.QueryParameter, error) { + var out []bigquery.QueryParameter + if err := json.Unmarshal(bytes, &out); err != nil { + return nil, fmt.Errorf("error decoding JSON: %w", err) + } + + return out, nil +} Review Comment:  Using `json.Marshal` and `json.Unmarshal` for `bigquery.QueryParameter` will cause type loss for the `Value interface{}` field. For example, any integer value (like `int` or `int64`) will be unmarshaled as a `float64`, which will cause BigQuery to fail with type mismatch errors (e.g., when comparing an `INT64` column with a `FLOAT64` parameter). Similarly, `time.Time` or `civil.Date` will be unmarshaled as `string`, which may also cause query failures. Consider using `gob` encoding instead, and registering the common types (like `time.Time`, `civil.Date`, `civil.DateTime`, and `civil.Time`) using `gob.Register` so that type fidelity is preserved across the serialization boundary. ```go import ( "bytes" "encoding/gob" "fmt" "time" "cloud.google.com/go/bigquery" "cloud.google.com/go/civil" ) func init() { gob.Register(time.Time{}) gob.Register(civil.Date{}) gob.Register(civil.DateTime{}) gob.Register(civil.Time{}) } func encodeQueryParameters(params []bigquery.QueryParameter) ([]byte, error) { var buf bytes.Buffer enc := gob.NewEncoder(&buf) if err := enc.Encode(params); err != nil { return nil, fmt.Errorf("error encoding gob: %w", err) } return buf.Bytes(), nil } func decodeQueryParameters(b []byte) ([]bigquery.QueryParameter, error) { var out []bigquery.QueryParameter dec := gob.NewDecoder(bytes.NewReader(b)) if err := dec.Decode(&out); err != nil { return nil, fmt.Errorf("error decoding gob: %w", err) } return out, nil } ``` ########## sdks/go/pkg/beam/io/bigqueryio/bigquery.go: ########## @@ -167,6 +189,11 @@ func (f *queryFn) ProcessElement(ctx context.Context, _ []byte, emit func(beam.X if !f.Options.UseStandardSQL { q.UseLegacySQL = true } + parameters, err := decodeQueryParameters(f.QueryParameters) + if err != nil { + return errors.Wrapf(err, "bigqueryio.queryFn: failed to decode query parameters") + } + q.Parameters = parameters Review Comment:  If `f.QueryParameters` is empty or `nil` (which is the case when no parameters are set, or for backward compatibility with older serialized pipelines), calling `decodeQueryParameters` directly will fail with an error (e.g., `unexpected end of JSON input` or `EOF` with gob). We should guard this call and only decode if `f.QueryParameters` is not empty. ```suggestion if len(f.QueryParameters) > 0 { parameters, err := decodeQueryParameters(f.QueryParameters) if err != nil { return errors.Wrapf(err, "bigqueryio.queryFn: failed to decode query parameters") } q.Parameters = parameters } ``` ########## sdks/go/pkg/beam/io/bigqueryio/bigquery.go: ########## @@ -142,7 +153,16 @@ func query(s beam.Scope, project, query string, t reflect.Type, options ...func( } imp := beam.Impulse(s) - return beam.ParDo(s, &queryFn{Project: project, Query: query, Type: beam.EncodedType{T: t}, Options: queryOptions}, imp, beam.TypeDefinition{Var: beam.XType, T: t}) + queryParameters, err := encodeQueryParameters(queryOptions.parameters) + if err != nil { + panic(errors.Wrapf(err, "bigqueryio.Query: failed to encode query parameters")) + } Review Comment:  To avoid unnecessary serialization overhead when no query parameters are provided, we should only encode the parameters if the slice is not empty. ```go var queryParameters []byte if len(queryOptions.parameters) > 0 { var err error queryParameters, err = encodeQueryParameters(queryOptions.parameters) if err != nil { panic(errors.Wrapf(err, "bigqueryio.Query: failed to encode query parameters")) } } ``` ########## sdks/go/pkg/beam/io/bigqueryio/coder_test.go: ########## @@ -0,0 +1,62 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package bigqueryio + +import ( + "testing" + + "cloud.google.com/go/bigquery" + "github.com/google/go-cmp/cmp" +) + +func Test_encodeDecodeQueryOptions(t *testing.T) { + tests := []struct { + name string + val bigquery.QueryParameter + }{ + { + name: "Encode/decode QueryOptions with parameters", + val: bigquery.QueryParameter{ + Name: "key", + Value: "value", + }, + }, + { + name: "Encode/decode QueryOptions with nil parameter", + val: bigquery.QueryParameter{ + Name: "key", + Value: nil, + }, + }, + } Review Comment:  The current test cases only cover `string` and `nil` values, which hides the type loss issue of JSON serialization. Please add test cases for `int` and `float64` values to ensure type preservation. ```go tests := []struct { name string val bigquery.QueryParameter }{ { name: "Encode/decode QueryOptions with string parameter", val: bigquery.QueryParameter{ Name: "key", Value: "value", }, }, { name: "Encode/decode QueryOptions with int parameter", val: bigquery.QueryParameter{ Name: "key", Value: 42, }, }, { name: "Encode/decode QueryOptions with float parameter", val: bigquery.QueryParameter{ Name: "key", Value: 3.14, }, }, { name: "Encode/decode QueryOptions with nil parameter", val: bigquery.QueryParameter{ Name: "key", Value: nil, }, }, } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
