From 804fee8614bdacbc5bf0eb02fa3bdd1c89a01b3a Mon Sep 17 00:00:00 2001 From: Egon Elbre Date: Thu, 22 Aug 2024 16:05:00 +0300 Subject: [PATCH 1/3] perf(spanner): avoid using fmt.Errorf unnecessarily Fixed automatically with: go fmt -w -r "fmt.Errorf(s) -> errors.New(s)" . goimports -w . Updates #9749 --- spanner/client_benchmarks_test.go | 4 ++-- spanner/client_test.go | 2 +- spanner/integration_test.go | 2 +- spanner/row.go | 4 ++-- spanner/session_test.go | 7 ++++--- spanner/sessionclient_test.go | 5 +++-- spanner/spannertest/db_eval.go | 5 +++-- spanner/spannertest/db_query.go | 7 ++++--- spanner/spannertest/inmem.go | 5 +++-- spanner/value.go | 28 ++++++++++++++-------------- spanner/value_test.go | 3 ++- 11 files changed, 39 insertions(+), 33 deletions(-) diff --git a/spanner/client_benchmarks_test.go b/spanner/client_benchmarks_test.go index d24d4b9a5832..4a0a0714100b 100644 --- a/spanner/client_benchmarks_test.go +++ b/spanner/client_benchmarks_test.go @@ -18,7 +18,7 @@ package spanner import ( "context" - "fmt" + "errors" "math/rand" "reflect" "sync" @@ -87,7 +87,7 @@ func createBenchmarkServer(incStep uint64) (server *MockedSpannerInMemTestServer if uint64(client.idleSessions.idleList.Len()) == client.idleSessions.MinOpened { return nil } - return fmt.Errorf("not yet initialized") + return errors.New("not yet initialized") }) return } diff --git a/spanner/client_test.go b/spanner/client_test.go index 20891a5a1519..31c3f934cab6 100644 --- a/spanner/client_test.go +++ b/spanner/client_test.go @@ -1935,7 +1935,7 @@ func TestClient_ReadWriteTransaction_BufferedWriteBeforeSqlStatementWithError(t // We ignore the error and proceed to commit the transaction. _, err := tx.Update(ctx, NewStatement(UpdateBarSetFoo)) if err == nil { - return fmt.Errorf("missing expected InvalidArgument error") + return errors.New("missing expected InvalidArgument error") } return nil }) diff --git a/spanner/integration_test.go b/spanner/integration_test.go index ae2fd6f32bbe..2f60421a3841 100644 --- a/spanner/integration_test.go +++ b/spanner/integration_test.go @@ -2844,7 +2844,7 @@ func TestIntegration_StructTypes(t *testing.T) { return fmt.Errorf("len(rows) = %d; want 1", len(rows)) } if !rows[0].Valid { - return fmt.Errorf("rows[0] is NULL") + return errors.New("rows[0] is NULL") } var i, j int64 if err := rows[0].Row.Columns(&i, &j); err != nil { diff --git a/spanner/row.go b/spanner/row.go index 8bf0771fa8c2..b2be78a0a92c 100644 --- a/spanner/row.go +++ b/spanner/row.go @@ -418,10 +418,10 @@ func (r *Row) ToStructLenient(p interface{}) error { // err := spanner.SelectAll(row, &singersByPtr, spanner.WithLenient()) func SelectAll(rows rowIterator, destination interface{}, options ...DecodeOptions) error { if rows == nil { - return fmt.Errorf("rows is nil") + return errors.New("rows is nil") } if destination == nil { - return fmt.Errorf("destination is nil") + return errors.New("destination is nil") } dstVal := reflect.ValueOf(destination) if !dstVal.IsValid() || (dstVal.Kind() == reflect.Ptr && dstVal.IsNil()) { diff --git a/spanner/session_test.go b/spanner/session_test.go index 2cf12995abce..39d3c1c23202 100644 --- a/spanner/session_test.go +++ b/spanner/session_test.go @@ -20,6 +20,7 @@ import ( "bytes" "container/heap" "context" + "errors" "fmt" "math/rand" "reflect" @@ -279,7 +280,7 @@ func TestTakeFromIdleListChecked(t *testing.T) { numOpened := uint64(sp.idleList.Len()) sp.mu.Unlock() if numOpened < sp.SessionPoolConfig.incStep-1 { - return fmt.Errorf("creation not yet finished") + return errors.New("creation not yet finished") } return nil }) @@ -1900,7 +1901,7 @@ func TestMaintainer_DeletesSessions(t *testing.T) { sp.mu.Lock() defer sp.mu.Unlock() if sp.numOpened > 0 { - return fmt.Errorf("session pool still contains more than 0 sessions") + return errors.New("session pool still contains more than 0 sessions") } return nil }) @@ -2023,7 +2024,7 @@ func TestSessionCreationIsDistributedOverChannels(t *testing.T) { numOpened := uint64(sp.idleList.Len()) sp.mu.Unlock() if numOpened < spc.MinOpened { - return fmt.Errorf("not yet initialized") + return errors.New("not yet initialized") } return nil }) diff --git a/spanner/sessionclient_test.go b/spanner/sessionclient_test.go index d1813754267b..ce2ea8f161fe 100644 --- a/spanner/sessionclient_test.go +++ b/spanner/sessionclient_test.go @@ -18,6 +18,7 @@ package spanner import ( "context" + "errors" "fmt" "sync" "testing" @@ -260,7 +261,7 @@ func TestBatchCreateAndCloseSession(t *testing.T) { client.idleSessions.mu.Lock() defer client.idleSessions.mu.Unlock() if client.idleSessions.multiplexedSession == nil { - return fmt.Errorf("multiplexed session not created yet") + return errors.New("multiplexed session not created yet") } return nil }) @@ -475,7 +476,7 @@ func TestBatchCreateSessions_ServerExhausted(t *testing.T) { if isMultiplexEnabled { waitFor(t, func() error { if client.idleSessions.multiplexedSession == nil { - return fmt.Errorf("multiplexed session not created yet") + return errors.New("multiplexed session not created yet") } return nil }) diff --git a/spanner/spannertest/db_eval.go b/spanner/spannertest/db_eval.go index a46e511a7b18..928efb9c44d4 100644 --- a/spanner/spannertest/db_eval.go +++ b/spanner/spannertest/db_eval.go @@ -20,6 +20,7 @@ package spannertest import ( "bytes" + "errors" "fmt" "regexp" "strconv" @@ -284,7 +285,7 @@ func (ec evalContext) evalArithOp(e spansql.ArithOp) (interface{}, error) { } if rhs == 0 { // TODO: Does real Spanner use a specific error code here? - return nil, fmt.Errorf("divide by zero") + return nil, errors.New("divide by zero") } return lhs / rhs, nil case spansql.Add, spansql.Sub, spansql.Mul: @@ -916,7 +917,7 @@ func (ec evalContext) colInfo(e spansql.Expr) (colInfo, error) { return colInfo{}, err } if ci.Type.Array { - return colInfo{}, fmt.Errorf("can't nest array literals") + return colInfo{}, errors.New("can't nest array literals") } ci.Type.Array = true return ci, nil diff --git a/spanner/spannertest/db_query.go b/spanner/spannertest/db_query.go index fce95134bd47..902d70a0cca4 100644 --- a/spanner/spannertest/db_query.go +++ b/spanner/spannertest/db_query.go @@ -17,6 +17,7 @@ limitations under the License. package spannertest import ( + "errors" "fmt" "io" "sort" @@ -445,7 +446,7 @@ func (d *database) evalSelect(sel spansql.Select, qc *queryContext) (si *selIter // First stage is to identify the data source. // If there's a FROM then that names a table to use. if len(sel.From) > 1 { - return nil, fmt.Errorf("selecting with more than one FROM clause not yet supported") + return nil, errors.New("selecting with more than one FROM clause not yet supported") } if len(sel.From) == 1 { var err error @@ -751,11 +752,11 @@ func (d *database) evalSelectFrom(qc *queryContext, ec evalContext, sf spansql.S func newJoinIter(lhs, rhs *rawIter, lhsEC, rhsEC evalContext, sfj spansql.SelectFromJoin) (*joinIter, evalContext, error) { if sfj.On != nil && len(sfj.Using) > 0 { - return nil, evalContext{}, fmt.Errorf("JOIN may not have both ON and USING clauses") + return nil, evalContext{}, errors.New("JOIN may not have both ON and USING clauses") } if sfj.On == nil && len(sfj.Using) == 0 && sfj.Type != spansql.CrossJoin { // TODO: This isn't correct for joining against a non-table. - return nil, evalContext{}, fmt.Errorf("non-CROSS JOIN must have ON or USING clause") + return nil, evalContext{}, errors.New("non-CROSS JOIN must have ON or USING clause") } // Start with the context from the LHS (aliases and params should be the same on both sides). diff --git a/spanner/spannertest/inmem.go b/spanner/spannertest/inmem.go index 3a4763e52fab..104a26a7f25a 100644 --- a/spanner/spannertest/inmem.go +++ b/spanner/spannertest/inmem.go @@ -50,6 +50,7 @@ package spannertest import ( "context" "encoding/base64" + "errors" "fmt" "io" "log" @@ -629,10 +630,10 @@ func (s *server) StreamingRead(req *spannerpb.ReadRequest, stream spannerpb.Span } if len(req.ResumeToken) > 0 { // This should only happen if we send resume_token ourselves. - return fmt.Errorf("read resumption not supported") + return errors.New("read resumption not supported") } if len(req.PartitionToken) > 0 { - return fmt.Errorf("partition restrictions not supported") + return errors.New("partition restrictions not supported") } var ri rowIter diff --git a/spanner/value.go b/spanner/value.go index a02af38957d7..8870c7492099 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -222,7 +222,7 @@ func (n NullInt64) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullInt64. func (n *NullInt64) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Int64 = int64(0) @@ -302,7 +302,7 @@ func (n NullString) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullString. func (n *NullString) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.StringVal = "" @@ -387,7 +387,7 @@ func (n NullFloat64) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullFloat64. func (n *NullFloat64) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Float64 = float64(0) @@ -467,7 +467,7 @@ func (n NullFloat32) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullFloat32. func (n *NullFloat32) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Float32 = float32(0) @@ -547,7 +547,7 @@ func (n NullBool) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullBool. func (n *NullBool) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Bool = false @@ -627,7 +627,7 @@ func (n NullTime) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullTime. func (n *NullTime) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Time = time.Time{} @@ -712,7 +712,7 @@ func (n NullDate) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullDate. func (n *NullDate) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Date = civil.Date{} @@ -797,7 +797,7 @@ func (n NullNumeric) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullNumeric. func (n *NullNumeric) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Numeric = big.Rat{} @@ -894,7 +894,7 @@ func (n NullJSON) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullJSON. func (n *NullJSON) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Valid = false @@ -942,7 +942,7 @@ func (n PGNumeric) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for PGNumeric. func (n *PGNumeric) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Numeric = "" @@ -989,7 +989,7 @@ func (n NullProtoMessage) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullProtoMessage. func (n *NullProtoMessage) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.ProtoMessageVal = nil @@ -1035,7 +1035,7 @@ func (n NullProtoEnum) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullProtoEnum. func (n *NullProtoEnum) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.ProtoEnumVal = nil @@ -1096,7 +1096,7 @@ func (n PGJsonB) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for PGJsonB. func (n *PGJsonB) UnmarshalJSON(payload []byte) error { if payload == nil { - return fmt.Errorf("payload should not be nil") + return errors.New("payload should not be nil") } if jsonIsNull(payload) { n.Valid = false @@ -4512,7 +4512,7 @@ func convertCustomTypeValue(sourceType decodableSpannerType, v interface{}) (int var destination reflect.Value switch sourceType { case spannerTypeInvalid: - return nil, fmt.Errorf("cannot encode a value to type spannerTypeInvalid") + return nil, errors.New("cannot encode a value to type spannerTypeInvalid") case spannerTypeNonNullString: destination = reflect.Indirect(reflect.New(reflect.TypeOf(""))) case spannerTypeNullString: diff --git a/spanner/value_test.go b/spanner/value_test.go index b1b2e33e87f9..f20e9069c6b6 100644 --- a/spanner/value_test.go +++ b/spanner/value_test.go @@ -19,6 +19,7 @@ package spanner import ( "bytes" "encoding/json" + "errors" "fmt" "math" "math/big" @@ -211,7 +212,7 @@ func (c *customArray) DecodeSpanner(val interface{}) error { } asSlice := listVal.AsSlice() if len(asSlice) != 4 { - return fmt.Errorf("failed to decode customArray: expected array of length 4") + return errors.New("failed to decode customArray: expected array of length 4") } for i, vI := range asSlice { vStr, ok := vI.(string) From 04dd959dd1faa505789bf947d16b7c4c177f78f0 Mon Sep 17 00:00:00 2001 From: Egon Elbre Date: Thu, 22 Aug 2024 16:16:37 +0300 Subject: [PATCH 2/3] perf(spanner): avoid duplicated errors.New in UnmarshalJSON errors.New generates a significant amount of code, however, the error definition can be easily shared between the implementations. --- spanner/value.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/spanner/value.go b/spanner/value.go index 8870c7492099..609414e99454 100644 --- a/spanner/value.go +++ b/spanner/value.go @@ -119,6 +119,8 @@ var ( protoMsgReflectType = reflect.TypeOf((*proto.Message)(nil)).Elem() protoEnumReflectType = reflect.TypeOf((*protoreflect.Enum)(nil)).Elem() + + errPayloadNil = errors.New("payload should not be nil") ) // UseNumberWithJSONDecoderEncoder specifies whether Cloud Spanner JSON numbers are decoded @@ -222,7 +224,7 @@ func (n NullInt64) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullInt64. func (n *NullInt64) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Int64 = int64(0) @@ -302,7 +304,7 @@ func (n NullString) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullString. func (n *NullString) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.StringVal = "" @@ -387,7 +389,7 @@ func (n NullFloat64) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullFloat64. func (n *NullFloat64) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Float64 = float64(0) @@ -467,7 +469,7 @@ func (n NullFloat32) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullFloat32. func (n *NullFloat32) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Float32 = float32(0) @@ -547,7 +549,7 @@ func (n NullBool) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullBool. func (n *NullBool) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Bool = false @@ -627,7 +629,7 @@ func (n NullTime) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullTime. func (n *NullTime) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Time = time.Time{} @@ -712,7 +714,7 @@ func (n NullDate) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullDate. func (n *NullDate) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Date = civil.Date{} @@ -797,7 +799,7 @@ func (n NullNumeric) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullNumeric. func (n *NullNumeric) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Numeric = big.Rat{} @@ -894,7 +896,7 @@ func (n NullJSON) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullJSON. func (n *NullJSON) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Valid = false @@ -942,7 +944,7 @@ func (n PGNumeric) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for PGNumeric. func (n *PGNumeric) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Numeric = "" @@ -989,7 +991,7 @@ func (n NullProtoMessage) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullProtoMessage. func (n *NullProtoMessage) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.ProtoMessageVal = nil @@ -1035,7 +1037,7 @@ func (n NullProtoEnum) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for NullProtoEnum. func (n *NullProtoEnum) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.ProtoEnumVal = nil @@ -1096,7 +1098,7 @@ func (n PGJsonB) MarshalJSON() ([]byte, error) { // UnmarshalJSON implements json.Unmarshaler.UnmarshalJSON for PGJsonB. func (n *PGJsonB) UnmarshalJSON(payload []byte) error { if payload == nil { - return errors.New("payload should not be nil") + return errPayloadNil } if jsonIsNull(payload) { n.Valid = false From f7b3917041dfb9304df1099f69d4193ecb31618b Mon Sep 17 00:00:00 2001 From: Egon Elbre Date: Thu, 22 Aug 2024 16:25:06 +0300 Subject: [PATCH 3/3] fix(spanner): error strings should not be capitalized --- spanner/client.go | 2 +- spanner/spannertest/db_eval.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spanner/client.go b/spanner/client.go index 5d3d078a5b0d..259e245953ac 100644 --- a/spanner/client.go +++ b/spanner/client.go @@ -90,7 +90,7 @@ func validDatabaseName(db string) error { func parseDatabaseName(db string) (project, instance, database string, err error) { matches := validDBPattern.FindStringSubmatch(db) if len(matches) == 0 { - return "", "", "", fmt.Errorf("Failed to parse database name from %q according to pattern %q", + return "", "", "", fmt.Errorf("failed to parse database name from %q according to pattern %q", db, validDBPattern.String()) } return matches[1], matches[2], matches[3], nil diff --git a/spanner/spannertest/db_eval.go b/spanner/spannertest/db_eval.go index 928efb9c44d4..552cb4a532c3 100644 --- a/spanner/spannertest/db_eval.go +++ b/spanner/spannertest/db_eval.go @@ -715,7 +715,7 @@ func (ec evalContext) evalExtractExpr(expr spansql.ExtractExpr) (result interfac return int64(v.Year), nil } } - return nil, fmt.Errorf("Extract with part %v not supported", expr.Part) + return nil, fmt.Errorf("extract with part %v not supported", expr.Part) } func (ec evalContext) evalAtTimeZoneExpr(expr spansql.AtTimeZoneExpr) (result interface{}, err error) {