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

@@ -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
 

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)

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

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)
 			}

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

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
 		}

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
 		}

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)
 	}
 }

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)

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.

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)

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)

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.

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 == "-"
 

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"

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)
 	}
 }

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

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")

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")

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 {

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)

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,
 				}

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{}
 }

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
 	}

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

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":

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

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)
 		}

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"`
 }

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,

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),

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 {

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

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) {

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.

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)
 }
 

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

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]
 	}

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()