From d2856bfeee0d3c24af75f4ad20af814b7ec5304d Mon Sep 17 00:00:00 2001 From: Travis Turner Date: Fri, 10 Mar 2023 15:13:15 -0600 Subject: [PATCH] Linters! (#2314) * Add (commented out) linters that we should introduce I went through the available linters and added (commented out) the ones I think we should work on in the near term. In other words, fix them, then uncomment them so they are enabled in CI. * linter: errchkjson * linter: ineffassign * linter: gosimple * linter: errname --- .golangci.yml | 18 +++++++++++++++++- api_test.go | 2 +- batch/batch_test.go | 4 ++-- batch/egpool/egpool.go | 6 +++--- cmd/root.go | 2 +- ctl/backup.go | 6 +++--- ctl/backup_tar.go | 4 ++-- ctl/backup_test.go | 6 +++--- ctl/dataframe-csv-loader.go | 2 +- ctl/export.go | 4 ++-- ctl/import.go | 6 +++--- ctl/parquet-info.go | 9 ++++----- ctl/restore.go | 4 ++-- ctl/restore_tar.go | 2 +- ctl/restore_tar_test.go | 2 +- ctl/restore_test.go | 4 ++-- ctl/util.go | 2 +- dax/queryer/orchestrator.go | 4 ++-- executor.go | 7 +++---- fragment.go | 1 - http_handler.go | 10 +++++----- idalloc.go | 8 ++++---- idk/api/source.go | 2 +- idk/cmd/molecula-consumer-csv/main.go | 2 +- idk/idallocator.go | 2 +- idk/ingest.go | 4 ++-- idk/interfaces.go | 12 +++++------- idk/internal/s3.go | 8 ++++---- idk/kafka/csrc/csrc.go | 2 +- idk/kafka/source.go | 24 ++++++++++++------------ idk/kinesis/reader.go | 2 +- rbf/rbf.go | 2 +- roaring/container_archetypes.go | 6 +++--- roaring/roaring.go | 2 +- server/server.go | 6 +++--- sql/query.go | 6 +++++- sql3/planner/opquery.go | 8 ++++++-- sql3/planner/types/operator.go | 5 +---- sql3/test/helpers.go | 5 ++++- 39 files changed, 115 insertions(+), 96 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index cfd527473..a39132ee0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -10,9 +10,25 @@ run: - pql/pql.peg.go linters: enable: + # Recommended to be enabled by default (https://golangci-lint.run). + # - errcheck (lots to fix) + - gosimple - govet - - gofmt + - ineffassign - staticcheck + - typecheck + # - unused (about 20 to fix) + + # Additional linters we choose to enable. + # - bodyclose (lots to fix, but we should) + - errchkjson + - errname + - gofmt + # - misspell (lots to fix, but we should) + # - prealloc (15 to fix) + # - predeclared (20 to fix) + # - stylecheck (quite a lot to fix, but we should definitely work on this) + # - unconvert (not at all critical, but makes for cleaner code) enable-all: false disable-all: true diff --git a/api_test.go b/api_test.go index 9718f0ac6..7875aa378 100644 --- a/api_test.go +++ b/api_test.go @@ -837,7 +837,7 @@ func TestAPI_IDAlloc(t *testing.T) { t.Fatalf("obtaining random bytes: %v", err) } ids3, err := primary.ReserveIDs(key, session, 0, 2) - var esync pilosa.ErrIDOffsetDesync + var esync pilosa.IDOffsetDesyncError if errors.As(err, &esync) { if esync.Requested != 0 { t.Errorf("incorrect requested offset in error: provided %d but got %d", 0, esync.Requested) diff --git a/batch/batch_test.go b/batch/batch_test.go index a07a3b573..bcc5a65c0 100644 --- a/batch/batch_test.go +++ b/batch/batch_test.go @@ -2045,7 +2045,7 @@ func mutexClearRegression(t *testing.T, importer featurebase.Importer, sapi feat } col := uint64(0) - row := uint64(1) + row := uint64(0) for i := uint64(0); i <= 21; i++ { col = (i%2+1)*featurebase.ShardWidth + i%5 row = i % 3 @@ -2126,7 +2126,7 @@ func mutexNilClearID(t *testing.T, importer featurebase.Importer, sapi featureba } col := uint64(0) - row := uint64(1) + row := uint64(0) // populate mutex with some data for i := uint64(0); i < 11; i++ { col = (i%2+1)*featurebase.ShardWidth + i%5 diff --git a/batch/egpool/egpool.go b/batch/egpool/egpool.go index 51d6d0c5b..03602e743 100644 --- a/batch/egpool/egpool.go +++ b/batch/egpool/egpool.go @@ -58,11 +58,11 @@ func (eg *Group) err(err error) { eg.errs = append(eg.errs, err) } -type ErrPanic struct { +type PanicError struct { Value interface{} } -func (p ErrPanic) Error() string { +func (p PanicError) Error() string { return fmt.Sprintf("panic: %v", p.Value) } @@ -77,7 +77,7 @@ func (eg *Group) processJobs() { defer func() { if !finished { if p := recover(); p != nil { - eg.err(ErrPanic{p}) + eg.err(PanicError{p}) } else { eg.err(ErrGoexit) } diff --git a/cmd/root.go b/cmd/root.go index d05956e9f..00ba0cba1 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -41,7 +41,7 @@ func UsageErrorWrapper(inner runner) func(*cobra.Command, []string) error { // wrappers. func considerUsageError(cmd *cobra.Command, err error) error { cmd.SilenceErrors = true - if !errors.Is(err, ctl.UsageError) { + if !errors.Is(err, ctl.ErrUsage) { cmd.SilenceUsage = true } return err diff --git a/ctl/backup.go b/ctl/backup.go index 248630759..86406a1e2 100644 --- a/ctl/backup.go +++ b/ctl/backup.go @@ -94,13 +94,13 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) { // Validate arguments. if cmd.OutputDir == "" { - return fmt.Errorf("%w: -o flag required", UsageError) + return fmt.Errorf("%w: -o flag required", ErrUsage) } else if cmd.Concurrency <= 0 { - return fmt.Errorf("%w: concurrency must be at least one", UsageError) + return fmt.Errorf("%w: concurrency must be at least one", ErrUsage) } if cmd.HeaderTimeoutStr != "" { if dur, err := time.ParseDuration(cmd.HeaderTimeoutStr); err != nil { - return fmt.Errorf("%w: could not parse '%s' as a duration: %v", UsageError, cmd.HeaderTimeoutStr, err) + return fmt.Errorf("%w: could not parse '%s' as a duration: %v", ErrUsage, cmd.HeaderTimeoutStr, err) } else { cmd.HeaderTimeout = dur } diff --git a/ctl/backup_tar.go b/ctl/backup_tar.go index bd07ded09..37abf3616 100644 --- a/ctl/backup_tar.go +++ b/ctl/backup_tar.go @@ -82,7 +82,7 @@ func (cmd *BackupTarCommand) Run(ctx context.Context) (err error) { logdest := cmd.Logger() // Validate arguments. if cmd.OutputPath == "" { - return fmt.Errorf("%w: -o flag required", UsageError) + return fmt.Errorf("%w: -o flag required", ErrUsage) } useStdout := cmd.OutputPath == "-" if useStdout && cmd.logwriter == os.Stdout { @@ -100,7 +100,7 @@ func (cmd *BackupTarCommand) Run(ctx context.Context) (err error) { if cmd.HeaderTimeoutStr != "" { if dur, err := time.ParseDuration(cmd.HeaderTimeoutStr); err != nil { - return fmt.Errorf("%w: could not parse '%s' as a duration: %v", UsageError, cmd.HeaderTimeoutStr, err) + return fmt.Errorf("%w: could not parse '%s' as a duration: %v", ErrUsage, cmd.HeaderTimeoutStr, err) } else { cmd.HeaderTimeout = dur } diff --git a/ctl/backup_test.go b/ctl/backup_test.go index 98cc7b74b..1d02e15fe 100644 --- a/ctl/backup_test.go +++ b/ctl/backup_test.go @@ -15,19 +15,19 @@ func TestBackupCommand_Run(t *testing.T) { cm := NewBackupCommand(cmLog) cm.OutputDir = "" err := cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error, got %v", err) } cm.OutputDir = "foo" cm.Concurrency = 0 err = cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error, got %v", err) } cm.Concurrency = 1 cm.HeaderTimeoutStr = "until the cat wakes up" err = cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error, got %v", err) } } diff --git a/ctl/dataframe-csv-loader.go b/ctl/dataframe-csv-loader.go index 2a5f893dc..3a8f0dff4 100644 --- a/ctl/dataframe-csv-loader.go +++ b/ctl/dataframe-csv-loader.go @@ -149,7 +149,7 @@ func (cmd *DataframeCsvLoaderCommand) Run(ctx context.Context) (err error) { // Validate arguments. if cmd.Path == "" { - return fmt.Errorf("%w: --csv flag required", UsageError) + return fmt.Errorf("%w: --csv flag required", ErrUsage) } readFile, err := os.Open(cmd.Path) diff --git a/ctl/export.go b/ctl/export.go index 7007ba68e..c2683ed10 100644 --- a/ctl/export.go +++ b/ctl/export.go @@ -50,9 +50,9 @@ func (cmd *ExportCommand) Run(ctx context.Context) error { // Validate arguments. if cmd.Index == "" { - return fmt.Errorf("%w: %v", UsageError, pilosa.ErrIndexRequired) + return fmt.Errorf("%w: %v", ErrUsage, pilosa.ErrIndexRequired) } else if cmd.Field == "" { - return fmt.Errorf("%w: %v", UsageError, pilosa.ErrFieldRequired) + return fmt.Errorf("%w: %v", ErrUsage, pilosa.ErrFieldRequired) } // Use output file, if specified. diff --git a/ctl/import.go b/ctl/import.go index 37c1981da..0911b8d2a 100644 --- a/ctl/import.go +++ b/ctl/import.go @@ -86,11 +86,11 @@ func (cmd *ImportCommand) Run(ctx context.Context) error { // Validate arguments. // Index and field are validated early before the files are parsed. if cmd.Index == "" { - return fmt.Errorf("%w: %v", UsageError, pilosa.ErrIndexRequired) + return fmt.Errorf("%w: %v", ErrUsage, pilosa.ErrIndexRequired) } else if cmd.Field == "" { - return fmt.Errorf("%w: %v", UsageError, pilosa.ErrFieldRequired) + return fmt.Errorf("%w: %v", ErrUsage, pilosa.ErrFieldRequired) } else if len(cmd.Paths) == 0 { - return fmt.Errorf("%w: path required", UsageError) + return fmt.Errorf("%w: path required", ErrUsage) } // Create a client to the server. client, err := commandClient(cmd) diff --git a/ctl/parquet-info.go b/ctl/parquet-info.go index 6e9125920..52b28e8ca 100644 --- a/ctl/parquet-info.go +++ b/ctl/parquet-info.go @@ -4,7 +4,6 @@ package ctl import ( "context" - "errors" "fmt" "io" "net/http" @@ -47,23 +46,23 @@ func (cmd *ParquetInfoCommand) Run(ctx context.Context) error { return err } if response.StatusCode != 200 { - return errors.New(fmt.Sprintf("unexpected response %d", response.StatusCode)) + return fmt.Errorf("unexpected response %d", response.StatusCode) } defer response.Body.Close() // download to temp file first f, err = os.CreateTemp("", "BulkParquetFile.parquet") if err != nil { - return errors.New(fmt.Sprintf("error creating tempfile %v", err)) + return fmt.Errorf("error creating tempfile %v", err) } _, err = io.Copy(f, response.Body) if err != nil { - return errors.New(fmt.Sprintf("error downloading url %v %v", cmd.Path, err)) + return fmt.Errorf("error downloading url %v %v", cmd.Path, err) } defer os.Remove(f.Name()) _, err = f.Seek(0, io.SeekStart) if err != nil { - return errors.New(fmt.Sprintf("error reseting file for reading %v ", err)) + return fmt.Errorf("error reseting file for reading %v ", err) } } else { f, err = os.Open(cmd.Path) diff --git a/ctl/restore.go b/ctl/restore.go index aed8dffa2..7b282e723 100644 --- a/ctl/restore.go +++ b/ctl/restore.go @@ -83,9 +83,9 @@ func (cmd *RestoreCommand) Run(ctx context.Context) (err error) { // Validate arguments. if cmd.Path == "" { - return fmt.Errorf("%w: -s flag required", UsageError) + return fmt.Errorf("%w: -s flag required", ErrUsage) } else if cmd.Concurrency <= 0 { - return fmt.Errorf("%w: concurrency must be at least one", UsageError) + return fmt.Errorf("%w: concurrency must be at least one", ErrUsage) } // Parse TLS configuration for node-specific clients. diff --git a/ctl/restore_tar.go b/ctl/restore_tar.go index be1052f99..034aae04a 100644 --- a/ctl/restore_tar.go +++ b/ctl/restore_tar.go @@ -80,7 +80,7 @@ func (cmd *RestoreTarCommand) Run(ctx context.Context) (err error) { // Validate arguments. if cmd.Path == "" { - return fmt.Errorf("%w: -s flag required", UsageError) + return fmt.Errorf("%w: -s flag required", ErrUsage) } useStdin := cmd.Path == "-" diff --git a/ctl/restore_tar_test.go b/ctl/restore_tar_test.go index 692a76386..8ddb9f45a 100644 --- a/ctl/restore_tar_test.go +++ b/ctl/restore_tar_test.go @@ -24,7 +24,7 @@ func TestRestoreTarCommand_Run(t *testing.T) { cm.Host = hostport cm.Path = "" err := cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error with empty path, got %v", err) } cm.Path = "nonexistent-file" diff --git a/ctl/restore_test.go b/ctl/restore_test.go index 69ac1baa6..9685939a8 100644 --- a/ctl/restore_test.go +++ b/ctl/restore_test.go @@ -15,13 +15,13 @@ func TestRestoreCommand_Run(t *testing.T) { cm := NewRestoreCommand(cmLog) cm.Path = "" err := cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error, got %v", err) } cm.Path = "foo" cm.Concurrency = 0 err = cm.Run(context.Background()) - if !errors.Is(err, UsageError) { + if !errors.Is(err, ErrUsage) { t.Fatalf("expected usage error, got %v", err) } } diff --git a/ctl/util.go b/ctl/util.go index ccb365704..c027b9ec9 100644 --- a/ctl/util.go +++ b/ctl/util.go @@ -19,7 +19,7 @@ func (c ctlUsageError) Error() string { return "usage error" } -var UsageError ctlUsageError +var ErrUsage ctlUsageError // startProfilingServer starts a server which handles /debug/pprof and // /debug/fgprof for use in utilities we might want to profile but diff --git a/dax/queryer/orchestrator.go b/dax/queryer/orchestrator.go index 9ff35381d..129912f7e 100644 --- a/dax/queryer/orchestrator.go +++ b/dax/queryer/orchestrator.go @@ -1186,8 +1186,8 @@ type Error string // TODO(jaffee) convert to standard error package func (e Error) Error() string { return string(e) } -const ViewNotFound = Error("view not found") -const FragmentNotFound = Error("fragment not found") +const ErrViewNotFound = Error("view not found") +const ErrFragmentNotFound = Error("fragment not found") func (o *orchestrator) executeTopK(ctx context.Context, tableKeyer dax.TableKeyer, c *pql.Call, shards []uint64, opt *featurebase.ExecOptions) (interface{}, error) { span, ctx := tracing.StartSpanFromContext(ctx, "Executor.executeTopK") diff --git a/executor.go b/executor.go index c63771c36..48ab171e2 100644 --- a/executor.go +++ b/executor.go @@ -1750,8 +1750,7 @@ func (d *DistinctTimestamp) Union(other DistinctTimestamp) DistinctTimestamp { } const ( - ViewNotFound = Error("view not found") - FragmentNotFound = Error("fragment not found") + ErrViewNotFound = Error("view not found") ) func executeDistinctShardSet(ctx context.Context, qcx *Qcx, idx *Index, fieldName string, shard uint64, filterBitmap *roaring.Bitmap) (result *Row, err0 error) { @@ -1764,7 +1763,7 @@ func executeDistinctShardSet(ctx context.Context, qcx *Qcx, idx *Index, fieldNam fragData, _, err := tx.ContainerIterator(index, fieldName, "standard", shard, 0) switch errors.Cause(err) { - case ViewNotFound, FragmentNotFound: + case ErrViewNotFound, ErrFragmentNotFound: // It may seem reasonable to return `nil` here in the case where the // fragment for this shard does not exist. The problem with doing that // is that if this operation is being performed on a remote node, then @@ -1851,7 +1850,7 @@ func executeDistinctShardBSI(ctx context.Context, qcx *Qcx, idx *Index, fieldNam existsBitmap, err := tx.OffsetRange(index, fieldName, view, shard, ShardWidth*shard, ShardWidth*0, ShardWidth*1) if err != nil { switch errors.Cause(err) { - case ViewNotFound, FragmentNotFound: + case ErrViewNotFound, ErrFragmentNotFound: return result, nil } return result, errors.Wrap(err, "getting exists bitmap") diff --git a/fragment.go b/fragment.go index 3ba4a2df9..5be779cb5 100644 --- a/fragment.go +++ b/fragment.go @@ -416,7 +416,6 @@ func (f *fragment) clearBit(tx Tx, rowID, columnID uint64) (changed bool, err er // unprotectedClearBit TODO should be replaced by an invocation of // importPositions with a single bit to clear. func (f *fragment) unprotectedClearBit(tx Tx, rowID, columnID uint64) (changed bool, err error) { - changed = false // Determine the position of the bit in the storage. pos, err := f.pos(rowID, columnID) if err != nil { diff --git a/http_handler.go b/http_handler.go index e77377350..cdfc4fe09 100644 --- a/http_handler.go +++ b/http_handler.go @@ -1476,7 +1476,7 @@ func (h *Handler) handlePostSQL(w http.ResponseWriter, r *http.Request) { // Write the closing bracket on any exit from this method. defer func() { - var execTime int64 = 0 + var execTime int64 // we are going to make best effort here - don't actually care about the error // if there was an error, the request.ElapsedTime will be zero request, _ := h.api.server.SystemLayer.ExecutionRequests().GetRequest(requestID.String()) @@ -3752,16 +3752,16 @@ func (h *Handler) handleReserveIDs(w http.ResponseWriter, r *http.Request) { ids, err := h.api.ReserveIDs(req.Key, req.Session, req.Offset, req.Count) if err != nil { - var esync ErrIDOffsetDesync + var esync IDOffsetDesyncError if errors.As(err, &esync) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusConflict) err = json.NewEncoder(w).Encode(struct { - ErrIDOffsetDesync + IDOffsetDesyncError Err string `json:"error"` }{ - ErrIDOffsetDesync: esync, - Err: err.Error(), + IDOffsetDesyncError: esync, + Err: err.Error(), }) if err != nil { h.logger.Debugf("failed to send desync error: %v", err) diff --git a/idalloc.go b/idalloc.go index 699dc411b..988156d2f 100644 --- a/idalloc.go +++ b/idalloc.go @@ -109,10 +109,10 @@ func (ida *idAllocator) WriteTo(w io.Writer) (int64, error) { return tx.WriteTo(w) } -// ErrIDOffsetDesync is an error generated when attempting to reserve IDs at a committed offset. +// IDOffsetDesyncError is an error generated when attempting to reserve IDs at a committed offset. // This will typically happen when kafka partitions are moved between kafka ingesters - there may be a brief period in which 2 ingesters are processing the same messages at the same time. // The ingester can resolve this by ignoring messages under base. -type ErrIDOffsetDesync struct { +type IDOffsetDesyncError struct { // Requested is the offset that the client attempted to reserve. Requested uint64 `json:"requested"` @@ -120,7 +120,7 @@ type ErrIDOffsetDesync struct { Base uint64 `json:"base"` } -func (err ErrIDOffsetDesync) Error() string { +func (err IDOffsetDesyncError) Error() string { return fmt.Sprintf("attempted to reserve IDs at committed offset %d (base offset: %d)", err.Requested, err.Base) } @@ -149,7 +149,7 @@ func (ida *idAllocator) reserve(key IDAllocKey, session [32]byte, offset, count if offset < res.offset { // This probbably means that 2 clients are running at the same time. // This is fine - just tell the client that is behind what had been dealt with. - return ErrIDOffsetDesync{ + return IDOffsetDesyncError{ Requested: offset, Base: res.offset, } diff --git a/idk/api/source.go b/idk/api/source.go index fc19f065d..29aa5a3aa 100644 --- a/idk/api/source.go +++ b/idk/api/source.go @@ -375,7 +375,7 @@ var friendlyTypeNames = map[reflect.Type]string{ } // ErrDuplicateElement is an error indicating that an element was included in a set multiple times. -type ErrDuplicateElement struct { +type ErrDuplicateElement struct { //nolint (should be: DuplicateElementError) // Elem is the duplicated element. Elem interface{} } diff --git a/idk/cmd/molecula-consumer-csv/main.go b/idk/cmd/molecula-consumer-csv/main.go index f56c287a5..ce379db2e 100644 --- a/idk/cmd/molecula-consumer-csv/main.go +++ b/idk/cmd/molecula-consumer-csv/main.go @@ -30,7 +30,7 @@ func main() { return } - if m.Concurrency != 1 && m.AutoGenerate == true { + if m.Concurrency != 1 && m.AutoGenerate { m.Log().Infof("Concurrency is not supported for csv ingest when using '--auto-generate'. '--concurrency' flag will be ignored and concurrency will be set to 1.") m.Concurrency = 1 } diff --git a/idk/idallocator.go b/idk/idallocator.go index 1ce11b53d..a8e07f2cd 100644 --- a/idk/idallocator.go +++ b/idk/idallocator.go @@ -329,7 +329,7 @@ type pilosaIDManager struct { // TODO, once pull/1559 is merged into pilosa main branch // no need to maintain a duplicate definition of ErrIDOffsetDesync // here -type ErrIDOffsetDesync struct { +type ErrIDOffsetDesync struct { //nolint Requested uint64 `json:"requested"` // Base is the next lowest uncommitted offset for which // IDs may be reserved diff --git a/idk/ingest.go b/idk/ingest.go index 7603f55f3..a3f0f7144 100644 --- a/idk/ingest.go +++ b/idk/ingest.go @@ -1463,7 +1463,7 @@ func (m *Main) runDeleter(limitCounter *msgCounter) error { for i, value := range rec.Data() { name := avroFields[i].Name if name == "_id" { - if m.index.Opts().Keys() == false { + if !m.index.Opts().Keys() { recordID, err = toUint64(value) if err != nil { return errors.Errorf("unable convert _id to uint64 for index %s which is has keys set to false", m.index.Name()) @@ -1559,7 +1559,7 @@ func (m *Main) runDeleter(limitCounter *msgCounter) error { m.log.Debugf("Delete consumer running the follow delete queries: %s", bq.Serialize()) resp, err := client.Query(bq, nil) - if err != nil || resp.Success != true { + if err != nil || !resp.Success { return errors.Wrap(err, "error deleting values") } case "records": diff --git a/idk/interfaces.go b/idk/interfaces.go index 07b4bcb1a..85679e957 100644 --- a/idk/interfaces.go +++ b/idk/interfaces.go @@ -1252,15 +1252,13 @@ func toStringArray(val interface{}) ([]string, error) { case []interface{}: ret := make([]string, len(vt)) for i, v := range vt { - switch v.(type) { + switch v := v.(type) { case []byte: - ret[i] = string(v.([]byte)[:]) + ret[i] = string(v[:]) + case string: + ret[i] = v default: - vs, ok := v.(string) - if !ok { - return nil, errors.Errorf("couldn't convert []interface{} to []string, value %v of type %[1]T at %d", v, i) - } - ret[i] = vs + return nil, errors.Errorf("couldn't convert []interface{} to []string, value %v of type %[1]T at %d", v, i) } } return ret, nil diff --git a/idk/internal/s3.go b/idk/internal/s3.go index ed6a3ea05..86ce7c1ed 100644 --- a/idk/internal/s3.go +++ b/idk/internal/s3.go @@ -13,7 +13,7 @@ import ( "github.com/pkg/errors" ) -var FileOrURLNotFound = errors.New("file or url does not exist") +var ErrFileOrURLNotFound = errors.New("file or url does not exist") // ReadFileOrURL reads a path from the filesystem or an s3 URL. // The s3client parameter is required if reading an s3 URL. @@ -41,9 +41,9 @@ func ReadFileOrURL(name string, s3client s3iface.S3API) ([]byte, error) { if aerr, ok := err.(awserr.Error); ok { switch aerr.Code() { case s3.ErrCodeNoSuchBucket: - return nil, FileOrURLNotFound + return nil, ErrFileOrURLNotFound case s3.ErrCodeNoSuchKey: - return nil, FileOrURLNotFound + return nil, ErrFileOrURLNotFound } } return nil, errors.Wrapf(err, "fetching S3 object %v", name) @@ -59,7 +59,7 @@ func ReadFileOrURL(name string, s3client s3iface.S3API) ([]byte, error) { content, err = os.ReadFile(name) if err != nil { if os.IsNotExist(err) { - return nil, FileOrURLNotFound + return nil, ErrFileOrURLNotFound } return nil, errors.Wrapf(err, "reading file %v", name) } diff --git a/idk/kafka/csrc/csrc.go b/idk/kafka/csrc/csrc.go index 85ae1b85a..25cd927c8 100644 --- a/idk/kafka/csrc/csrc.go +++ b/idk/kafka/csrc/csrc.go @@ -73,7 +73,7 @@ type SchemaResponse struct { ID int `json:"id"` // Registry's unique id } -type ErrorResponse struct { +type ErrorResponse struct { //nolint:errname StatusCode int `json:"error_code"` Body string `json:"message"` } diff --git a/idk/kafka/source.go b/idk/kafka/source.go index 2c1a848dd..8f458cf80 100644 --- a/idk/kafka/source.go +++ b/idk/kafka/source.go @@ -571,7 +571,7 @@ func avroToPDKField(aField *avro.SchemaField) (idk.Field, error) { switch ft { case "decimal": scale, err := intProp(aField, "scale") - if scale > 18 || err == wrongType { + if scale > 18 || err == errWrongType { return nil, errors.Errorf("0<=scale<=18, got:%d err:%v", scale, err) } return idk.DecimalField{ @@ -610,12 +610,12 @@ func avroToPDKField(aField *avro.SchemaField) (idk.Field, error) { }, nil case "timestamp": layout, err := stringProp(aField, "layout") - if err == wrongType { + if err == errWrongType { return nil, errors.Errorf("property provided in wrong type for TimestampField: layout, err:%v", err) } unit, err := stringProp(aField, "unit") - if err == wrongType { + if err == errWrongType { return nil, errors.Errorf("property provided in wrong type for TimestampField: unit, err:%v", err) } @@ -624,7 +624,7 @@ func avroToPDKField(aField *avro.SchemaField) (idk.Field, error) { } granularity, err := stringProp(aField, "granularity") - if err == wrongType { + if err == errWrongType { return nil, errors.Errorf("property provided in wrong type for TimestampField: unit, err:%v", err) } @@ -795,7 +795,7 @@ func avroToPDKField(aField *avro.SchemaField) (idk.Field, error) { NameVal: aField.Name, } scale, err := intProp(aField, "scale") - if err == wrongType { + if err == errWrongType { return nil, errors.Wrap(err, "getting scale") } else if err == nil { field.Scale = scale @@ -824,11 +824,11 @@ func avroToPDKField(aField *avro.SchemaField) (idk.Field, error) { func stringProp(p propper, s string) (string, error) { ival, ok := p.Prop(s) if !ok { - return "", notFound + return "", errNotFound } sval, ok := ival.(string) if !ok { - return "", wrongType + return "", errWrongType } return sval, nil } @@ -855,14 +855,14 @@ func cacheConfigProp(p propper) (*idk.CacheConfig, error) { func intProp(p propper, s string) (int64, error) { ival, ok := p.Prop(s) if !ok { - return 0, notFound + return 0, errNotFound } switch v := ival.(type) { case string: n, e := strconv.ParseInt(v, 10, 64) if e != nil { - return 0, errors.Wrap(e, wrongType.Error()) + return 0, errors.Wrap(e, errWrongType.Error()) } return n, nil @@ -876,7 +876,7 @@ func intProp(p propper, s string) (int64, error) { return int64(v), nil } - return 0, wrongType + return 0, errWrongType } type propper interface { @@ -884,8 +884,8 @@ type propper interface { } var ( - notFound = errors.New("prop not found") - wrongType = errors.New("val is wrong type") + errNotFound = errors.New("prop not found") + errWrongType = errors.New("val is wrong type") ) // avroUnionToPDKField takes an avro SchemaField with a Union type, diff --git a/idk/kinesis/reader.go b/idk/kinesis/reader.go index 00721cbb3..474efcc29 100644 --- a/idk/kinesis/reader.go +++ b/idk/kinesis/reader.go @@ -201,7 +201,7 @@ func (r *shardReader) getRecords() error { func ReadOffsets(cfg StreamReaderConfig) (*StreamOffsets, error) { offsetsBytes, err := internal.ReadFileOrURL(cfg.offsetsPath, cfg.s3client) if err != nil { - if err == internal.FileOrURLNotFound { + if err == internal.ErrFileOrURLNotFound { return &StreamOffsets{ StreamName: cfg.streamName, Shards: make(map[string]*ShardOffset), diff --git a/rbf/rbf.go b/rbf/rbf.go index 80901efba..790dbd1e2 100644 --- a/rbf/rbf.go +++ b/rbf/rbf.go @@ -810,7 +810,7 @@ func (m *Metric) Inc(d time.Duration) { } // ErrorList represents a list of errors. -type ErrorList []error +type ErrorList []error //nolint:errname // Err returns the list if it contains errors. Otherwise returns nil. func (a ErrorList) Err() error { diff --git a/roaring/container_archetypes.go b/roaring/container_archetypes.go index de1e0cc44..c3a30fa85 100644 --- a/roaring/container_archetypes.go +++ b/roaring/container_archetypes.go @@ -40,7 +40,7 @@ var ContainerArchetypeNames = []string{ } var containerArchetypes [][]*Container -var containerArchetypesErr error +var errContainerArchetypes error var initContainerArchetypes sync.Once func makeArchetypalContainer(rng *rand.Rand, name string) (*Container, error) { @@ -167,9 +167,9 @@ func makeArchetypalContainer(rng *rand.Rand, name string) (*Container, error) { // called, and returns the results of that one call. func InitContainerArchetypes() ([][]*Container, error) { initContainerArchetypes.Do(func() { - containerArchetypes, containerArchetypesErr = createContainerArchetypes(8) + containerArchetypes, errContainerArchetypes = createContainerArchetypes(8) }) - return containerArchetypes, containerArchetypesErr + return containerArchetypes, errContainerArchetypes } // createContainerArchetypes creates a slice of *roaring.Container corresponding diff --git a/roaring/roaring.go b/roaring/roaring.go index be2b06522..34c6b66df 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -6569,7 +6569,7 @@ func trailingZeroN(v uint64) int { } // ErrorList represents a list of errors. -type ErrorList []error +type ErrorList []error //nolint:errname func (a ErrorList) Error() string { switch len(a) { diff --git a/server/server.go b/server/server.go index 0076e770c..9f479f5c1 100644 --- a/server/server.go +++ b/server/server.go @@ -201,7 +201,7 @@ const ( // to report on whether or not that succeeded. var ( setupResourceLimitsOnce sync.Once - setupResourceLimitsErr error + errSetupResourceLimits error ) // doSetupResourceLimits is the function which actually does the @@ -264,9 +264,9 @@ func (m *Command) doSetupResourceLimits() error { // denied errors are not that concerning. func (m *Command) setupResourceLimits() error { setupResourceLimitsOnce.Do(func() { - setupResourceLimitsErr = m.doSetupResourceLimits() + errSetupResourceLimits = m.doSetupResourceLimits() }) - return setupResourceLimitsErr + return errSetupResourceLimits } // StartNoServe starts the pilosa server, but doesn't serve on the http handler. diff --git a/sql/query.go b/sql/query.go index 731fc32cb..9ce3b0686 100644 --- a/sql/query.go +++ b/sql/query.go @@ -5,6 +5,7 @@ package sql import ( "encoding/json" "fmt" + "log" "strconv" "strings" "time" @@ -218,7 +219,10 @@ func ConstRow(ids ...interface{}) string { if ids == nil { ids = []interface{}{} } - data, _ := json.Marshal(ids) + data, err := json.Marshal(ids) + if err != nil { + log.Printf("marshalling json: %s", err) + } return fmt.Sprintf("ConstRow(columns=%s)", data) } diff --git a/sql3/planner/opquery.go b/sql3/planner/opquery.go index 6b34a2327..a38b54cf2 100644 --- a/sql3/planner/opquery.go +++ b/sql3/planner/opquery.go @@ -10,6 +10,7 @@ import ( fbcontext "github.com/featurebasedb/featurebase/v3/context" "github.com/featurebasedb/featurebase/v3/sql3" "github.com/featurebasedb/featurebase/v3/sql3/planner/types" + "github.com/pkg/errors" ) // PlanOpQuery is a query - this is the root node of an execution plan @@ -130,10 +131,13 @@ func (i *queryIterator) Next(ctx context.Context) (types.Row, error) { // either error or no more rows, either way update the request requestId, ok := fbcontext.RequestID(ctx) if !ok { - return nil, sql3.NewErrInternalf("unable to get request id from context") + return nil, errors.Wrapf(sql3.NewErrInternalf("unable to get request id from context"), "next on child: %s", err) } - plan, _ := json.MarshalIndent(i.query.Plan(), "", " ") + plan, err := json.MarshalIndent(i.query.Plan(), "", " ") + if err != nil { + i.query.planner.logger.Infof("marshal indent: %s", err) + } i.requests.UpdateRequest(requestId, time.Now(), "complete", "", 0, "", 0, 0, 0, 0, 0, string(plan)) } return row, err diff --git a/sql3/planner/types/operator.go b/sql3/planner/types/operator.go index a0d70cc7e..d2159136b 100644 --- a/sql3/planner/types/operator.go +++ b/sql3/planner/types/operator.go @@ -91,10 +91,7 @@ type Rows []Row // Append appends all the values in r2 to this row and returns the result func (r Row) Append(r2 Row) Row { row := make(Row, len(r)+len(r2)) - // TODO(pok) use a copy here - for i := range r { - row[i] = r[i] - } + copy(row, r) for i := range r2 { row[i+len(r)] = r2[i] } diff --git a/sql3/test/helpers.go b/sql3/test/helpers.go index b576ef36d..9e7b62389 100644 --- a/sql3/test/helpers.go +++ b/sql3/test/helpers.go @@ -30,7 +30,10 @@ func MustQueryRows(tb testing.TB, svr *featurebase.Server, q string) ([][]interf // get the plan so that code runs during testing plan := stmt.Plan() - bplan, _ := json.MarshalIndent(plan, "", " ") + bplan, err := json.MarshalIndent(plan, "", " ") + if err != nil { + return nil, nil, nil, err + } ocolumns := stmt.Schema()