From b84e70726f2a66f6dd451d67406614519ba8e71c Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Wed, 12 Aug 2020 20:37:53 +0200 Subject: [PATCH 1/2] *: ensure that log and error messages start with lowercase This commit adjusts a few log and error calls to ensure the texts start with a lowercase letter. (Messages that start with a code identifier keep their capital if any - these are treated as proper nouns.) Good: `log.Infof(ctx, "this is my message: %s", err)` Less good: `log.Info(ctx, "This is my message: %s", err)` Good: `errors.Wrap(err, "some context")` Less good: `errors.Wrap(err, "Some context")` This aligns the code with the prevalent convention valid throughout the project. The reason for adjusting this now is that log entries will soon receive additional scrutiny from users in the shape of documentation. We want the UX to be consistent throughout all logging and error outputs. A linter may be introduced later to enforce this convention. For context, why we use lowercase: Both log and error messages are always reported to users in a text format after a colon, for example, `file.go:123: this is the log message`. In English, random capital letters are not allowed in the middle of sentences (unlike in German) and distract from the flow of reading. This is particularly important for error objects, which can receive an arbitrary amount of text prefixes using `errors.Wrap()` in callers. Additionally, error objects can be included inside a larger sentence, for example `while handling "this is my error"`. Here, as well, capitals are not welcome after an opening quote in the middle of a sentence. Release note: None --- pkg/ccl/cmdccl/enc_utils/main.go | 2 +- pkg/cli/debug.go | 2 +- pkg/cli/demo_cluster.go | 2 +- pkg/cli/node.go | 4 +-- pkg/cli/quit.go | 2 +- pkg/cli/systembench/disk_bench.go | 2 +- pkg/geo/geohash.go | 2 +- pkg/gossip/gossip.go | 2 +- pkg/jobs/job_scheduler.go | 4 +-- pkg/jobs/jobs.go | 18 ++++++------- pkg/jobs/registry.go | 2 +- pkg/jobs/update.go | 6 ++--- pkg/kv/bulk/buffering_adder.go | 4 ++- pkg/kv/kvserver/tenantrate/limiter.go | 2 +- pkg/rpc/context.go | 2 +- pkg/security/certs.go | 26 +++++++++---------- pkg/server/authentication.go | 2 +- pkg/server/node.go | 2 +- pkg/server/status/recorder.go | 2 +- pkg/server/status/runtime.go | 2 +- pkg/server/updates.go | 4 +-- pkg/sql/app_stats.go | 2 +- pkg/sql/backfill.go | 4 +-- pkg/sql/catalog/lease/lease.go | 2 +- pkg/sql/check.go | 6 ++--- pkg/sql/colfetcher/cfetcher.go | 8 +++--- pkg/sql/delegate/show_partitions.go | 2 +- pkg/sql/distsql_running.go | 6 ++--- pkg/sql/flowinfra/outbox.go | 4 +-- .../testdata/logic_test/builtin_function | 2 +- .../logictest/testdata/logic_test/geospatial | 2 +- pkg/sql/opt/memo/check_expr.go | 4 +-- pkg/sql/pg_catalog.go | 2 +- pkg/sql/physicalplan/fake_span_resolver.go | 2 +- pkg/sql/rowexec/stream_merger.go | 2 +- pkg/sql/scrub.go | 2 +- pkg/sql/sem/builtins/math_builtins.go | 2 +- pkg/sql/sqlbase/column_type_encoding.go | 2 +- pkg/sql/sqlbase/encoded_datum.go | 2 +- pkg/sql/sqlliveness/slinstance/slinstance.go | 6 ++--- .../sqlliveness/slinstance/slinstance_test.go | 2 +- pkg/sql/sqlliveness/slstorage/slstorage.go | 6 ++--- pkg/sql/sqlliveness/slstorage/test_helpers.go | 2 +- pkg/sql/txn_state.go | 4 +-- pkg/sqlmigrations/migrations.go | 6 ++--- pkg/storage/cloudimpl/nodelocal_storage.go | 2 +- pkg/util/log/doc.go | 6 ++--- pkg/util/metric/registry.go | 10 +++---- pkg/util/randutil/rand.go | 2 +- 49 files changed, 99 insertions(+), 97 deletions(-) diff --git a/pkg/ccl/cmdccl/enc_utils/main.go b/pkg/ccl/cmdccl/enc_utils/main.go index 85333a0b1599..1d7d24da0177 100644 --- a/pkg/ccl/cmdccl/enc_utils/main.go +++ b/pkg/ccl/cmdccl/enc_utils/main.go @@ -90,7 +90,7 @@ func loadFileRegistry() { func loadStoreKey() { if len(*storeKeyPath) == 0 || *storeKeyPath == "plain" { - log.Infof(context.Background(), "No store key specified") + log.Infof(context.Background(), "no store key specified") return } diff --git a/pkg/cli/debug.go b/pkg/cli/debug.go index 63025b48d55c..98ff15bee76c 100644 --- a/pkg/cli/debug.go +++ b/pkg/cli/debug.go @@ -996,7 +996,7 @@ func removeDeadReplicas( removeDeadReplicasOpts.deadStoreIDs) if _, ok := deadStoreIDs[storeIdent.StoreID]; ok { - return nil, errors.Errorf("This store's ID (%s) marked as dead, aborting", storeIdent.StoreID) + return nil, errors.Errorf("this store's ID (%s) marked as dead, aborting", storeIdent.StoreID) } var newDescs []roachpb.RangeDescriptor diff --git a/pkg/cli/demo_cluster.go b/pkg/cli/demo_cluster.go index 11d2e7dac325..6e22c4a90c57 100644 --- a/pkg/cli/demo_cluster.go +++ b/pkg/cli/demo_cluster.go @@ -699,7 +699,7 @@ func (c *transientCluster) runWorkload( // Only log an error and return when the workload function throws // an error, because errors these errors should be ignored, and // should not interrupt the rest of the demo. - log.Warningf(ctx, "Error running workload query: %+v\n", err) + log.Warningf(ctx, "error running workload query: %+v", err) return } } diff --git a/pkg/cli/node.go b/pkg/cli/node.go index 171d4e70d03b..264bf5dec5cf 100644 --- a/pkg/cli/node.go +++ b/pkg/cli/node.go @@ -321,7 +321,7 @@ func runDecommissionNode(cmd *cobra.Command, args []string) error { conn, _, finish, err := getClientGRPCConn(ctx, serverCfg) if err != nil { - return errors.Wrap(err, "Failed to connect to the node") + return errors.Wrap(err, "failed to connect to the node") } defer finish() @@ -544,7 +544,7 @@ func runRecommissionNode(cmd *cobra.Command, args []string) error { conn, _, finish, err := getClientGRPCConn(ctx, serverCfg) if err != nil { - return errors.Wrap(err, "Failed to connect to the node") + return errors.Wrap(err, "failed to connect to the node") } defer finish() diff --git a/pkg/cli/quit.go b/pkg/cli/quit.go index 57d42b71b607..8b3fbdbd6d74 100644 --- a/pkg/cli/quit.go +++ b/pkg/cli/quit.go @@ -248,7 +248,7 @@ func doShutdown(ctx context.Context, c serverpb.AdminClient) (hardError bool, er func getAdminClient(ctx context.Context, cfg server.Config) (serverpb.AdminClient, func(), error) { conn, _, finish, err := getClientGRPCConn(ctx, cfg) if err != nil { - return nil, nil, errors.Wrap(err, "Failed to connect to the node") + return nil, nil, errors.Wrap(err, "failed to connect to the node") } return serverpb.NewAdminClient(conn), finish, nil } diff --git a/pkg/cli/systembench/disk_bench.go b/pkg/cli/systembench/disk_bench.go index 662a0f28cfb3..2ab63da07419 100644 --- a/pkg/cli/systembench/disk_bench.go +++ b/pkg/cli/systembench/disk_bench.go @@ -69,7 +69,7 @@ func newWorkerSeqWrite( data := make([]byte, diskOptions.WriteSize) _, err := rand.Read(data) if err != nil { - log.Fatalf(ctx, "Failed to fill byte array with random bytes %s", err) + log.Fatalf(ctx, "failed to fill byte array with random bytes %s", err) } w := &workerSeqWrite{data: data, diff --git a/pkg/geo/geohash.go b/pkg/geo/geohash.go index 3b028129874f..3164e04a714c 100644 --- a/pkg/geo/geohash.go +++ b/pkg/geo/geohash.go @@ -19,7 +19,7 @@ import ( // using a Lng/Lat Point representation of the GeoHash. func NewGeometryPointFromGeoHash(g string, precision int) (*Geometry, error) { if len(g) == 0 { - return nil, errors.Newf("Length of geohash must be greater than 0") + return nil, errors.Newf("length of geohash must be greater than 0") } // If precision is more than the length of the geohash diff --git a/pkg/gossip/gossip.go b/pkg/gossip/gossip.go index e30ae0ccfa19..8748a95e668c 100644 --- a/pkg/gossip/gossip.go +++ b/pkg/gossip/gossip.go @@ -388,7 +388,7 @@ func NewTestWithLocality( // AssertNotStarted fatals if the Gossip instance was already started. func (g *Gossip) AssertNotStarted(ctx context.Context) { if g.started { - log.Fatalf(ctx, "Gossip instance was already started") + log.Fatalf(ctx, "gossip instance was already started") } } diff --git a/pkg/jobs/job_scheduler.go b/pkg/jobs/job_scheduler.go index e1752d2c1d0d..680d13567809 100644 --- a/pkg/jobs/job_scheduler.go +++ b/pkg/jobs/job_scheduler.go @@ -277,7 +277,7 @@ func (s *jobScheduler) executeSchedules( func (s *jobScheduler) runDaemon(ctx context.Context, stopper *stop.Stopper) { stopper.RunWorker(ctx, func(ctx context.Context) { initialDelay := getInitialScanDelay(s.TestingKnobs) - log.Infof(ctx, "Waiting %s before scheduled jobs daemon start", initialDelay.String()) + log.Infof(ctx, "waiting %s before scheduled jobs daemon start", initialDelay.String()) for timer := time.NewTimer(initialDelay); ; timer.Reset( getWaitPeriod(&s.Settings.SV, s.TestingKnobs)) { @@ -286,7 +286,7 @@ func (s *jobScheduler) runDaemon(ctx context.Context, stopper *stop.Stopper) { return case <-timer.C: if !schedulerEnabledSetting.Get(&s.Settings.SV) { - log.Info(ctx, "Scheduled job daemon disabled") + log.Info(ctx, "scheduled job daemon disabled") continue } diff --git a/pkg/jobs/jobs.go b/pkg/jobs/jobs.go index 1457bd1167fb..83e2bc88985e 100644 --- a/pkg/jobs/jobs.go +++ b/pkg/jobs/jobs.go @@ -327,8 +327,8 @@ func (j *Job) FractionProgressed(ctx context.Context, progressedFn FractionProgr } if fractionCompleted < 0.0 || fractionCompleted > 1.0 { return errors.Errorf( - "Job: fractionCompleted %f is outside allowable range [0.0, 1.0] (job %d)", - fractionCompleted, *j.ID(), + "job %d: fractionCompleted %f is outside allowable range [0.0, 1.0]", + *j.ID(), fractionCompleted, ) } md.Progress.Progress = &jobspb.Progress_FractionCompleted{ @@ -353,8 +353,8 @@ func (j *Job) HighWaterProgressed(ctx context.Context, progressedFn HighWaterPro } if highWater.Less(hlc.Timestamp{}) { return errors.Errorf( - "Job: high-water %s is outside allowable range > 0.0 (job %d)", - highWater, *j.ID(), + "job %d: high-water %s is outside allowable range > 0.0", + *j.ID(), highWater, ) } md.Progress.Progress = &jobspb.Progress_HighWater{ @@ -591,7 +591,7 @@ func (j *Job) succeeded(ctx context.Context, fn func(context.Context, *kv.Txn) e return nil } if md.Status != StatusRunning && md.Status != StatusPending { - return errors.Errorf("Job with status %s cannot be marked as succeeded", md.Status) + return errors.Errorf("job with status %s cannot be marked as succeeded", md.Status) } if fn != nil { if err := fn(ctx, txn); err != nil { @@ -807,7 +807,7 @@ func UnmarshalPayload(datum tree.Datum) (*jobspb.Payload, error) { bytes, ok := datum.(*tree.DBytes) if !ok { return nil, errors.Errorf( - "Job: failed to unmarshal payload as DBytes (was %T)", datum) + "job: failed to unmarshal payload as DBytes (was %T)", datum) } if err := protoutil.Unmarshal([]byte(*bytes), payload); err != nil { return nil, err @@ -822,7 +822,7 @@ func UnmarshalProgress(datum tree.Datum) (*jobspb.Progress, error) { bytes, ok := datum.(*tree.DBytes) if !ok { return nil, errors.Errorf( - "Job: failed to unmarshal Progress as DBytes (was %T)", datum) + "job: failed to unmarshal Progress as DBytes (was %T)", datum) } if err := protoutil.Unmarshal([]byte(*bytes), progress); err != nil { return nil, err @@ -841,10 +841,10 @@ func unmarshalCreatedBy(createdByType, createdByID tree.Datum) (*CreatedByInfo, return &CreatedByInfo{Name: string(*ds), ID: int64(*id)}, nil } return nil, errors.Errorf( - "Job: failed to unmarshal created_by_type as DInt (was %T)", createdByID) + "job: failed to unmarshal created_by_type as DInt (was %T)", createdByID) } return nil, errors.Errorf( - "Job: failed to unmarshal created_by_type as DString (was %T)", createdByType) + "job: failed to unmarshal created_by_type as DString (was %T)", createdByType) } // CurrentStatus returns the current job status from the jobs table or error. diff --git a/pkg/jobs/registry.go b/pkg/jobs/registry.go index f1ab2e95d55c..21b11cf11aee 100644 --- a/pkg/jobs/registry.go +++ b/pkg/jobs/registry.go @@ -603,7 +603,7 @@ func (r *Registry) Start( log.Errorf(ctx, "error getting live session: %s", err) return } - log.VEventf(ctx, 1, "Registry live claim (instance_id: %s, sid: %s)", r.ID(), s.ID()) + log.VEventf(ctx, 1, "registry live claim (instance_id: %s, sid: %s)", r.ID(), s.ID()) if _, err := r.ex.QueryRowEx( ctx, "expire-sessions", nil, diff --git a/pkg/jobs/update.go b/pkg/jobs/update.go index 3c433336fa40..52a25c67384b 100644 --- a/pkg/jobs/update.go +++ b/pkg/jobs/update.go @@ -99,7 +99,7 @@ func (ju *JobUpdater) hasUpdates() bool { // defined in jobs.go. func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error { if j.id == nil { - return errors.New("Job: cannot update: job not created") + return errors.New("job: cannot update: job not created") } var payload *jobspb.Payload @@ -118,7 +118,7 @@ func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error { statusString, ok := row[0].(*tree.DString) if !ok { - return errors.Errorf("Job: expected string status on job %d, but got %T", *j.id, statusString) + return errors.AssertionFailedf("job %d: expected string status, but got %T", *j.id, statusString) } status := Status(*statusString) if payload, err = UnmarshalPayload(row[1]); err != nil { @@ -194,7 +194,7 @@ func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error { } if n != 1 { return errors.Errorf( - "Job: expected exactly one row affected, but %d rows affected by job update", n, + "job %d: expected exactly one row affected, but %d rows affected by job update", *j.id, n, ) } return nil diff --git a/pkg/kv/bulk/buffering_adder.go b/pkg/kv/bulk/buffering_adder.go index 6e76fc9af6b0..7f8d9f72b4d0 100644 --- a/pkg/kv/bulk/buffering_adder.go +++ b/pkg/kv/bulk/buffering_adder.go @@ -128,7 +128,9 @@ func MakeBulkAdder( // it will store in-memory before sending it to RocksDB. b.memAcc = bulkMon.MakeBoundAccount() if err := b.memAcc.Grow(ctx, b.curBufferSize); err != nil { - return nil, errors.Wrap(err, "Not enough memory available to create a BulkAdder. Try setting a higher --max-sql-memory.") + return nil, errors.WithHint( + errors.Wrap(err, "not enough memory available to create a BulkAdder"), + "Try setting a higher --max-sql-memory.") } return b, nil diff --git a/pkg/kv/kvserver/tenantrate/limiter.go b/pkg/kv/kvserver/tenantrate/limiter.go index 4ca3405d80bc..0c33cc162ead 100644 --- a/pkg/kv/kvserver/tenantrate/limiter.go +++ b/pkg/kv/kvserver/tenantrate/limiter.go @@ -195,7 +195,7 @@ func (rb *tokenBuckets) Merge(val interface{}) (shouldNotify bool) { // further in the future. return false default: - panic(errors.AssertionFailedf("Merge not implemented for %T", val)) + panic(errors.AssertionFailedf("merge not implemented for %T", val)) } } diff --git a/pkg/rpc/context.go b/pkg/rpc/context.go index 7f292f858f3c..73917a08b448 100644 --- a/pkg/rpc/context.go +++ b/pkg/rpc/context.go @@ -952,7 +952,7 @@ func (ctx *Context) grpcDialRaw( dialerFunc := dialer.dial if ctx.Knobs.ArtificialLatencyMap != nil { latency := ctx.Knobs.ArtificialLatencyMap[target] - log.VEventf(ctx.masterCtx, 1, "Connecting to node %s (%d) with simulated latency %dms", target, remoteNodeID, + log.VEventf(ctx.masterCtx, 1, "connecting to node %s (%d) with simulated latency %dms", target, remoteNodeID, latency) dialer := artificialLatencyDialer{ dialerFunc: dialerFunc, diff --git a/pkg/security/certs.go b/pkg/security/certs.go index f8488107c106..03dc2a577fe2 100644 --- a/pkg/security/certs.go +++ b/pkg/security/certs.go @@ -181,7 +181,7 @@ func createCACertAndKey( return errors.Errorf("could not write CA key to file %s: %v", caKeyPath, err) } - log.Infof(context.Background(), "Generated CA key %s", caKeyPath) + log.Infof(context.Background(), "generated CA key %s", caKeyPath) } else { if !allowKeyReuse { return errors.Errorf("CA key %s exists, but key reuse is disabled", caKeyPath) @@ -197,7 +197,7 @@ func createCACertAndKey( return errors.Errorf("could not parse CA key file %s: %v", caKeyPath, err) } - log.Infof(context.Background(), "Using CA key from file %s", caKeyPath) + log.Infof(context.Background(), "using CA key from file %s", caKeyPath) } // Generate certificate. @@ -233,7 +233,7 @@ func createCACertAndKey( if err != nil { return errors.Errorf("could not parse existing CA cert file %s: %v", certPath, err) } - log.Infof(context.Background(), "Found %d certificates in %s", + log.Infof(context.Background(), "found %d certificates in %s", len(existingCertificates), certPath) } else if !os.IsNotExist(err) { return errors.Errorf("could not stat CA cert file %s: %v", certPath, err) @@ -247,7 +247,7 @@ func createCACertAndKey( return errors.Errorf("could not write CA certificate file %s: %v", certPath, err) } - log.Infof(context.Background(), "Wrote %d certificates to %s", len(certificates), certPath) + log.Infof(context.Background(), "wrote %d certificates to %s", len(certificates), certPath) return nil } @@ -300,13 +300,13 @@ func CreateNodePair( if err := writeCertificateToFile(certPath, nodeCert, overwrite); err != nil { return errors.Errorf("error writing node server certificate to %s: %v", certPath, err) } - log.Infof(context.Background(), "Generated node certificate: %s", certPath) + log.Infof(context.Background(), "generated node certificate: %s", certPath) keyPath := cm.NodeKeyPath() if err := writeKeyToFile(keyPath, nodeKey, overwrite); err != nil { return errors.Errorf("error writing node server key to %s: %v", keyPath, err) } - log.Infof(context.Background(), "Generated node key: %s", keyPath) + log.Infof(context.Background(), "generated node key: %s", keyPath) return nil } @@ -355,13 +355,13 @@ func CreateUIPair( if err := writeCertificateToFile(certPath, uiCert, overwrite); err != nil { return errors.Errorf("error writing UI server certificate to %s: %v", certPath, err) } - log.Infof(context.Background(), "Generated UI certificate: %s", certPath) + log.Infof(context.Background(), "generated UI certificate: %s", certPath) keyPath := cm.UIKeyPath() if err := writeKeyToFile(keyPath, uiKey, overwrite); err != nil { return errors.Errorf("error writing UI server key to %s: %v", keyPath, err) } - log.Infof(context.Background(), "Generated UI key: %s", keyPath) + log.Infof(context.Background(), "generated UI key: %s", keyPath) return nil } @@ -426,20 +426,20 @@ func CreateClientPair( if err := writeCertificateToFile(certPath, clientCert, overwrite); err != nil { return errors.Errorf("error writing client certificate to %s: %v", certPath, err) } - log.Infof(context.Background(), "Generated client certificate: %s", certPath) + log.Infof(context.Background(), "generated client certificate: %s", certPath) keyPath := cm.ClientKeyPath(user) if err := writeKeyToFile(keyPath, clientKey, overwrite); err != nil { return errors.Errorf("error writing client key to %s: %v", keyPath, err) } - log.Infof(context.Background(), "Generated client key: %s", keyPath) + log.Infof(context.Background(), "generated client key: %s", keyPath) if wantPKCS8Key { pkcs8KeyPath := keyPath + ".pk8" if err := writePKCS8KeyToFile(pkcs8KeyPath, clientKey, overwrite); err != nil { return errors.Errorf("error writing client PKCS8 key to %s: %v", pkcs8KeyPath, err) } - log.Infof(context.Background(), "Generated PKCS8 client key: %s", pkcs8KeyPath) + log.Infof(context.Background(), "generated PKCS8 client key: %s", pkcs8KeyPath) } return nil @@ -523,13 +523,13 @@ func WriteTenantClientPair(certsDir string, cp *TenantClientPair, overwrite bool if err := writeCertificateToFile(certPath, cp.Cert, overwrite); err != nil { return errors.Errorf("error writing tenant certificate to %s: %v", certPath, err) } - log.Infof(context.Background(), "Wrote SQL tenant client certificate: %s", certPath) + log.Infof(context.Background(), "wrote SQL tenant client certificate: %s", certPath) keyPath := cm.TenantClientKeyPath(tenantIdentifier) if err := writeKeyToFile(keyPath, cp.PrivateKey, overwrite); err != nil { return errors.Errorf("error writing tenant key to %s: %v", keyPath, err) } - log.Infof(context.Background(), "Generated tenant key: %s", keyPath) + log.Infof(context.Background(), "generated tenant key: %s", keyPath) return nil } diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index d885b8e6dcdc..ac59b40ed86d 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -394,7 +394,7 @@ func (am *authenticationMux) ServeHTTP(w http.ResponseWriter, req *http.Request) ctx = context.WithValue(ctx, webSessionIDKey{}, cookie.ID) req = req.WithContext(ctx) } else if !am.allowAnonymous { - log.Infof(req.Context(), "Web session error: %s", err) + log.Infof(req.Context(), "web session error: %s", err) http.Error(w, "a valid authentication cookie is required", http.StatusUnauthorized) return } diff --git a/pkg/server/node.go b/pkg/server/node.go index d3026315859b..48d5ba30f6bd 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -356,7 +356,7 @@ func (n *Node) start( select { case <-n.storeCfg.Gossip.Connected: default: - log.Fatalf(ctx, "Gossip is not connected yet") + log.Fatalf(ctx, "gossip is not connected yet") } ctxWithSpan, span := n.AnnotateCtxWithSpan(ctx, "alloc-node-id") newID, err := allocateNodeID(ctxWithSpan, n.storeCfg.DB) diff --git a/pkg/server/status/recorder.go b/pkg/server/status/recorder.go index de406c0e423c..c2ba3381423e 100644 --- a/pkg/server/status/recorder.go +++ b/pkg/server/status/recorder.go @@ -464,7 +464,7 @@ func (mr *MetricsRecorder) GenerateNodeStatus(ctx context.Context) *statuspb.Nod // Gather descriptor from store. descriptor, err := mr.mu.stores[storeID].Descriptor(ctx, false /* useCached */) if err != nil { - log.Errorf(ctx, "Could not record status summaries: Store %d could not return descriptor, error: %s", storeID, err) + log.Errorf(ctx, "could not record status summaries: Store %d could not return descriptor, error: %s", storeID, err) continue } diff --git a/pkg/server/status/runtime.go b/pkg/server/status/runtime.go index b3c616d0f4d2..45d1e95b1ad7 100644 --- a/pkg/server/status/runtime.go +++ b/pkg/server/status/runtime.go @@ -307,7 +307,7 @@ func NewRuntimeStatSampler(ctx context.Context, clock *hlc.Clock) *RuntimeStatSa timestamp, err := info.Timestamp() if err != nil { // We can't panic here, tests don't have a build timestamp. - log.Warningf(ctx, "Could not parse build timestamp: %v", err) + log.Warningf(ctx, "could not parse build timestamp: %v", err) } // Build information. diff --git a/pkg/server/updates.go b/pkg/server/updates.go index ddea9b05fce3..8ba1d8dbd08f 100644 --- a/pkg/server/updates.go +++ b/pkg/server/updates.go @@ -227,7 +227,7 @@ func (s *Server) checkForUpdates(ctx context.Context) bool { err = decoder.Decode(&r) if err != nil && err != io.EOF { - log.Warningf(ctx, "Error decoding updates info: %v", err) + log.Warningf(ctx, "error decoding updates info: %v", err) return false } @@ -238,7 +238,7 @@ func (s *Server) checkForUpdates(ctx context.Context) bool { r.Details = r.Details[len(r.Details)-updateMaxVersionsToReport:] } for _, v := range r.Details { - log.Infof(ctx, "A new version is available: %s, details: %s", v.Version, v.Details) + log.Infof(ctx, "a new version is available: %s, details: %s", v.Version, v.Details) } return true } diff --git a/pkg/sql/app_stats.go b/pkg/sql/app_stats.go index 69ec2eb30fd3..ec30c7a8e67b 100644 --- a/pkg/sql/app_stats.go +++ b/pkg/sql/app_stats.go @@ -550,7 +550,7 @@ func dumpStmtStats(ctx context.Context, appName string, stats map[stmtKey]*stmtS } fmt.Fprintf(&buf, "%q: %s\n", key.String(), json) } - log.Infof(ctx, "Statistics for %q:\n%s", appName, buf.String()) + log.Infof(ctx, "statistics for %q:\n%s", appName, buf.String()) } func constructStatementIDFromStmtKey(key stmtKey) roachpb.StmtID { diff --git a/pkg/sql/backfill.go b/pkg/sql/backfill.go index 6d63332aa325..876784c2f4f0 100644 --- a/pkg/sql/backfill.go +++ b/pkg/sql/backfill.go @@ -177,7 +177,7 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error { } version := tableDesc.Version - log.Infof(ctx, "Running backfill for %q, v=%d", tableDesc.Name, tableDesc.Version) + log.Infof(ctx, "running backfill for %q, v=%d", tableDesc.Name, tableDesc.Version) needColumnBackfill := false for _, m := range tableDesc.Mutations { @@ -303,7 +303,7 @@ func (sc *SchemaChanger) runBackfill(ctx context.Context) error { } } - log.Infof(ctx, "Completed backfill for %q, v=%d", tableDesc.Name, tableDesc.Version) + log.Infof(ctx, "completed backfill for %q, v=%d", tableDesc.Name, tableDesc.Version) if sc.testingKnobs.RunAfterBackfill != nil { if err := sc.testingKnobs.RunAfterBackfill(*sc.job.ID()); err != nil { diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go index 7db65d99007e..140e1b68503a 100644 --- a/pkg/sql/catalog/lease/lease.go +++ b/pkg/sql/catalog/lease/lease.go @@ -1353,7 +1353,7 @@ func (c *nameCache) get( defer desc.mu.Unlock() if !NameMatchesDescriptor(desc, parentID, parentSchemaID, name) { - panic(errors.AssertionFailedf("Out of sync entry in the name cache. "+ + panic(errors.AssertionFailedf("out of sync entry in the name cache. "+ "Cache entry: (%d, %d, %q) -> %d. Lease: (%d, %d, %q).", parentID, parentSchemaID, name, desc.GetID(), diff --git a/pkg/sql/check.go b/pkg/sql/check.go index 56f9055d163c..d5e6c825b8b8 100644 --- a/pkg/sql/check.go +++ b/pkg/sql/check.go @@ -51,7 +51,7 @@ func validateCheckExpr( colSelectors := sqlbase.ColumnsSelectors(tableDesc.Columns) columns := tree.AsStringWithFlags(&colSelectors, tree.FmtSerializable) queryStr := fmt.Sprintf(`SELECT %s FROM [%d AS t] WHERE NOT (%s) LIMIT 1`, columns, tableDesc.GetID(), exprStr) - log.Infof(ctx, "Validating check constraint %q with query %q", expr, queryStr) + log.Infof(ctx, "validating check constraint %q with query %q", expr, queryStr) rows, err := ie.QueryRow(ctx, "validate check constraint", txn, queryStr) if err != nil { @@ -259,7 +259,7 @@ func validateForeignKey( return err } - log.Infof(ctx, "Validating MATCH FULL FK %q (%q [%v] -> %q [%v]) with query %q", + log.Infof(ctx, "validating MATCH FULL FK %q (%q [%v] -> %q [%v]) with query %q", fk.Name, srcTable.Name, colNames, targetTable.GetName(), referencedColumnNames, @@ -285,7 +285,7 @@ func validateForeignKey( return err } - log.Infof(ctx, "Validating FK %q (%q [%v] -> %q [%v]) with query %q", + log.Infof(ctx, "validating FK %q (%q [%v] -> %q [%v]) with query %q", fk.Name, srcTable.Name, colNames, targetTable.GetName(), referencedColumnNames, query, diff --git a/pkg/sql/colfetcher/cfetcher.go b/pkg/sql/colfetcher/cfetcher.go index 2cb9952a4cda..5fae0909d9e9 100644 --- a/pkg/sql/colfetcher/cfetcher.go +++ b/pkg/sql/colfetcher/cfetcher.go @@ -712,7 +712,7 @@ func (rf *cFetcher) nextBatch(ctx context.Context) (coldata.Batch, error) { var foundNull bool if rf.mustDecodeIndexKey || rf.traceKV { if debugState { - log.Infof(ctx, "Decoding first key %s", rf.machine.nextKV.Key) + log.Infof(ctx, "decoding first key %s", rf.machine.nextKV.Key) } var ( key []byte @@ -741,7 +741,7 @@ func (rf *cFetcher) nextBatch(ctx context.Context) (coldata.Batch, error) { // We found an interleave. Set our skip prefix. seekPrefix := rf.machine.nextKV.Key[:len(key)+rf.table.knownPrefixLength] if debugState { - log.Infof(ctx, "Setting seek prefix to %s", seekPrefix) + log.Infof(ctx, "setting seek prefix to %s", seekPrefix) } rf.machine.seekPrefix = seekPrefix rf.machine.state[0] = stateSeekPrefix @@ -868,7 +868,7 @@ func (rf *cFetcher) nextBatch(ctx context.Context) (coldata.Batch, error) { // prefix and indicate that it needs decoding. rf.machine.nextKV = kv if debugState { - log.Infof(ctx, "Decoding next key %s", rf.machine.nextKV.Key) + log.Infof(ctx, "decoding next key %s", rf.machine.nextKV.Key) } // TODO(jordan): optimize this prefix check by skipping span prefix. @@ -1305,7 +1305,7 @@ func (rf *cFetcher) fillNulls() error { indexColValues = append(indexColValues, "?") } return scrub.WrapError(scrub.UnexpectedNullValueError, errors.Errorf( - "Non-nullable column \"%s:%s\" with no value! Index scanned was %q with the index key columns (%s) and the values (%s)", + "non-nullable column \"%s:%s\" with no value! Index scanned was %q with the index key columns (%s) and the values (%s)", table.desc.GetName(), table.cols[i].Name, table.index.Name, strings.Join(table.index.ColumnNames, ","), strings.Join(indexColValues, ","))) } diff --git a/pkg/sql/delegate/show_partitions.go b/pkg/sql/delegate/show_partitions.go index ac4f82f9ec3d..b4fe7b99990b 100644 --- a/pkg/sql/delegate/show_partitions.go +++ b/pkg/sql/delegate/show_partitions.go @@ -113,7 +113,7 @@ func (d *delegator) delegateShowPartitions(n *tree.ShowPartitions) (tree.Stateme if tn.ObjectName == "" { err := errors.New("no table specified") err = pgerror.WithCandidateCode(err, pgcode.InvalidParameterValue) - err = errors.WithHint(err, "Specify a table using the hint syntax of table@index") + err = errors.WithHint(err, "Specify a table using the hint syntax of table@index.") return nil, err } diff --git a/pkg/sql/distsql_running.go b/pkg/sql/distsql_running.go index 74942216b1c0..1a830a8a12a9 100644 --- a/pkg/sql/distsql_running.go +++ b/pkg/sql/distsql_running.go @@ -372,9 +372,9 @@ func (dsp *DistSQLPlanner) Run( } _, url, err := execinfrapb.GeneratePlanDiagramURL(stmtStr, flows, false /* showInputTypes */) if err != nil { - log.Infof(ctx, "Error generating diagram: %s", err) + log.Infof(ctx, "error generating diagram: %s", err) } else { - log.Infof(ctx, "Plan diagram URL:\n%s", url.String()) + log.Infof(ctx, "plan diagram URL:\n%s", url.String()) } } @@ -419,7 +419,7 @@ func (dsp *DistSQLPlanner) Run( // TODO(radu): this should go through the flow scheduler. if err := flow.Run(ctx, func() {}); err != nil { - log.Fatalf(ctx, "unexpected error from syncFlow.Start(): %s "+ + log.Fatalf(ctx, "unexpected error from syncFlow.Start(): %v\n"+ "The error should have gone to the consumer.", err) } diff --git a/pkg/sql/flowinfra/outbox.go b/pkg/sql/flowinfra/outbox.go index 1525082dd8da..f9771a38d10b 100644 --- a/pkg/sql/flowinfra/outbox.go +++ b/pkg/sql/flowinfra/outbox.go @@ -411,10 +411,10 @@ func (m *Outbox) listenForDrainSignalFromConsumer(ctx context.Context) (<-chan d return } case signal.SetupFlowRequest != nil: - log.Fatalf(ctx, "Unexpected SetupFlowRequest. "+ + log.Fatalf(ctx, "unexpected SetupFlowRequest.\n"+ "This SyncFlow specific message should have been handled in RunSyncFlow.") case signal.Handshake != nil: - log.Eventf(ctx, "Consumer sent handshake. Consuming flow scheduled: %t", + log.Eventf(ctx, "consumer sent handshake.\nConsuming flow scheduled: %t", signal.Handshake.ConsumerScheduled) } } diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function index b22612f6ec4a..ddaf5f028a11 100644 --- a/pkg/sql/logictest/testdata/logic_test/builtin_function +++ b/pkg/sql/logictest/testdata/logic_test/builtin_function @@ -2420,7 +2420,7 @@ SELECT width_bucket(now(), array['yesterday', 'today', 'tomorrow']::timestamptz[ ---- 2 -query error pq: width_bucket\(\): Operand and thresholds must be of the same type +query error pq: width_bucket\(\): operand and thresholds must be of the same type SELECT width_bucket(1, array['a', 'h', 'l', 'z']); # Regression for #40623 diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial b/pkg/sql/logictest/testdata/logic_test/geospatial index ff9db07e3714..05f8fcca7d6b 100644 --- a/pkg/sql/logictest/testdata/logic_test/geospatial +++ b/pkg/sql/logictest/testdata/logic_test/geospatial @@ -830,7 +830,7 @@ POINT (90 0) statement error pq: st_pointfromgeohash\(\): geohash decode '----': invalid character at index 0 SELECT ST_AsText(ST_PointFromGeoHash('----')) -statement error pq: st_pointfromgeohash\(\): Length of geohash must be greater than 0 +statement error pq: st_pointfromgeohash\(\): length of geohash must be greater than 0 SELECT ST_AsText(ST_PointFromGeoHash('')) query TTTTTTTT diff --git a/pkg/sql/opt/memo/check_expr.go b/pkg/sql/opt/memo/check_expr.go index f5e61c3c5721..0a47ef350ad0 100644 --- a/pkg/sql/opt/memo/check_expr.go +++ b/pkg/sql/opt/memo/check_expr.go @@ -303,10 +303,10 @@ func checkFilters(filters FiltersExpr) { for _, item := range filters { if item.Condition.Op() == opt.RangeOp { if !item.scalar.TightConstraints { - panic(errors.AssertionFailedf("Range operator should always have tight constraints")) + panic(errors.AssertionFailedf("range operator should always have tight constraints")) } if item.scalar.OuterCols.Len() != 1 { - panic(errors.AssertionFailedf("Range operator should have exactly one outer col")) + panic(errors.AssertionFailedf("range operator should have exactly one outer col")) } } } diff --git a/pkg/sql/pg_catalog.go b/pkg/sql/pg_catalog.go index f24cfa1da3c3..a9ebb3358faa 100644 --- a/pkg/sql/pg_catalog.go +++ b/pkg/sql/pg_catalog.go @@ -1976,7 +1976,7 @@ CREATE TABLE pg_catalog.pg_operator ( leftType = tree.NewDOid(tree.DInt(params.Types()[0].Oid())) rightType = tree.NewDOid(tree.DInt(params.Types()[1].Oid())) default: - panic(errors.AssertionFailedf("Unexpected operator %s with %d params", + panic(errors.AssertionFailedf("unexpected operator %s with %d params", opName, params.Length())) } returnType := tree.NewDOid(tree.DInt(returnTyper(nil).Oid())) diff --git a/pkg/sql/physicalplan/fake_span_resolver.go b/pkg/sql/physicalplan/fake_span_resolver.go index 5da670ea658e..5be1d92a968d 100644 --- a/pkg/sql/physicalplan/fake_span_resolver.go +++ b/pkg/sql/physicalplan/fake_span_resolver.go @@ -82,7 +82,7 @@ func (fit *fakeSpanResolverIterator) Seek( // Scan the range and keep a list of all potential split keys. kvs, err := fit.txn.Scan(ctx, span.Key, span.EndKey, 0) if err != nil { - log.Errorf(ctx, "Error in fake span resolver scan: %s", err) + log.Errorf(ctx, "error in fake span resolver scan: %s", err) fit.err = err return } diff --git a/pkg/sql/rowexec/stream_merger.go b/pkg/sql/rowexec/stream_merger.go index 1e690fb1c03e..7e2d81df8d6f 100644 --- a/pkg/sql/rowexec/stream_merger.go +++ b/pkg/sql/rowexec/stream_merger.go @@ -179,7 +179,7 @@ func makeStreamMerger( } for i, ord := range leftOrdering { if ord.Direction != rightOrdering[i].Direction { - return streamMerger{}, errors.New("Ordering mismatch") + return streamMerger{}, errors.New("ordering mismatch") } } diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index a974560e31ae..9dd53ace38c8 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -255,7 +255,7 @@ func (n *scrubNode) startScrubTable( } n.run.checkQueue = append(n.run.checkQueue, constraintsToCheck...) default: - panic(errors.AssertionFailedf("Unhandled SCRUB option received: %+v", v)) + panic(errors.AssertionFailedf("unhandled SCRUB option received: %+v", v)) } } diff --git a/pkg/sql/sem/builtins/math_builtins.go b/pkg/sql/sem/builtins/math_builtins.go index 329d0dc44f78..4146a869dbae 100644 --- a/pkg/sql/sem/builtins/math_builtins.go +++ b/pkg/sql/sem/builtins/math_builtins.go @@ -563,7 +563,7 @@ var mathBuiltins = map[string]builtinDefinition{ thresholds := tree.MustBeDArray(args[1]) if !operand.ResolvedType().Equivalent(thresholds.ParamTyp) { - return tree.NewDInt(0), errors.New("Operand and thresholds must be of the same type") + return tree.NewDInt(0), errors.New("operand and thresholds must be of the same type") } for i, v := range thresholds.Array { diff --git a/pkg/sql/sqlbase/column_type_encoding.go b/pkg/sql/sqlbase/column_type_encoding.go index d163f3d3b53d..07920fa0b441 100644 --- a/pkg/sql/sqlbase/column_type_encoding.go +++ b/pkg/sql/sqlbase/column_type_encoding.go @@ -1318,7 +1318,7 @@ func datumTypeToArrayElementEncodingType(t *types.T) (encoding.Type, error) { case types.INetFamily: return encoding.IPAddr, nil default: - return 0, errors.Errorf("Don't know encoding type for %s", t) + return 0, errors.AssertionFailedf("no known encoding type for %s", t) } } diff --git a/pkg/sql/sqlbase/encoded_datum.go b/pkg/sql/sqlbase/encoded_datum.go index b94b549ec13f..4f484b453c2d 100644 --- a/pkg/sql/sqlbase/encoded_datum.go +++ b/pkg/sql/sqlbase/encoded_datum.go @@ -168,7 +168,7 @@ func EncDatumValueFromBufferWithOffsetsAndType( // DatumToEncDatum initializes an EncDatum with the given Datum. func DatumToEncDatum(ctyp *types.T, d tree.Datum) EncDatum { if d == nil { - panic(errors.AssertionFailedf("Cannot convert nil datum to EncDatum")) + panic(errors.AssertionFailedf("cannot convert nil datum to EncDatum")) } dTyp := d.ResolvedType() diff --git a/pkg/sql/sqlliveness/slinstance/slinstance.go b/pkg/sql/sqlliveness/slinstance/slinstance.go index 5d85b0a86e9d..8a820caaffef 100644 --- a/pkg/sql/sqlliveness/slinstance/slinstance.go +++ b/pkg/sql/sqlliveness/slinstance/slinstance.go @@ -131,7 +131,7 @@ func (l *Instance) createSession(ctx context.Context) (*session, error) { break } if everySecond.ShouldLog() { - log.Errorf(ctx, "Failed to create a session at %d-th attempt: %v", i, err.Error()) + log.Errorf(ctx, "failed to create a session at %d-th attempt: %v", i, err.Error()) } continue } @@ -140,7 +140,7 @@ func (l *Instance) createSession(ctx context.Context) (*session, error) { if err != nil { return nil, err } - log.Infof(ctx, "Created new SQL liveness session %s", s.ID()) + log.Infof(ctx, "created new SQL liveness session %s", s.ID()) return s, nil } @@ -214,7 +214,7 @@ func (l *Instance) heartbeatLoop(ctx context.Context) { continue } if log.V(2) { - log.Infof(ctx, "Extended SQL liveness session %s", s.ID()) + log.Infof(ctx, "extended SQL liveness session %s", s.ID()) } t.Reset(l.hb()) } diff --git a/pkg/sql/sqlliveness/slinstance/slinstance_test.go b/pkg/sql/sqlliveness/slinstance/slinstance_test.go index 1251f298c350..9e7bc4d6adbc 100644 --- a/pkg/sql/sqlliveness/slinstance/slinstance_test.go +++ b/pkg/sql/sqlliveness/slinstance/slinstance_test.go @@ -62,7 +62,7 @@ func TestSQLInstance(t *testing.T) { require.Equal(t, s1.ID(), s2.ID()) _ = fakeStorage.Delete(ctx, s2.ID()) - t.Logf("Deleted session %s", s2.ID()) + t.Logf("deleted session %s", s2.ID()) a, err = fakeStorage.IsAlive(ctx, s2.ID()) require.NoError(t, err) require.False(t, a) diff --git a/pkg/sql/sqlliveness/slstorage/slstorage.go b/pkg/sql/sqlliveness/slstorage/slstorage.go index f96ac29be7dc..98ead6b3bde5 100644 --- a/pkg/sql/sqlliveness/slstorage/slstorage.go +++ b/pkg/sql/sqlliveness/slstorage/slstorage.go @@ -189,7 +189,7 @@ func (s *Storage) fetchSession( ctx, "expire-single-session", txn, ` SELECT expiration FROM system.sqlliveness WHERE session_id = $1`, sid.UnsafeBytes(), ) - return errors.Wrapf(err, "Could not query session id: %s", sid) + return errors.Wrapf(err, "could not query session id: %s", sid) }); err != nil { return false, hlc.Timestamp{}, err } @@ -269,7 +269,7 @@ func (s *Storage) Insert( return err }); err != nil { s.metrics.WriteFailures.Inc(1) - return errors.Wrapf(err, "Could not insert session %s", sid) + return errors.Wrapf(err, "could not insert session %s", sid) } log.Infof(ctx, "inserted sqlliveness session %s", sid) s.metrics.WriteSuccesses.Inc(1) @@ -297,7 +297,7 @@ UPDATE system.sqlliveness SET expiration = $1 WHERE session_id = $2 RETURNING se s.metrics.WriteFailures.Inc(1) } if err != nil { - return false, errors.Wrapf(err, "Could not update session %s", sid) + return false, errors.Wrapf(err, "could not update session %s", sid) } s.metrics.WriteSuccesses.Inc(1) return sessionExists, nil diff --git a/pkg/sql/sqlliveness/slstorage/test_helpers.go b/pkg/sql/sqlliveness/slstorage/test_helpers.go index 0d42064612f6..23eb1115f217 100644 --- a/pkg/sql/sqlliveness/slstorage/test_helpers.go +++ b/pkg/sql/sqlliveness/slstorage/test_helpers.go @@ -51,7 +51,7 @@ func (s *FakeStorage) Insert( s.mu.Lock() defer s.mu.Unlock() if _, ok := s.mu.sessions[sid]; ok { - return errors.Errorf("Session %s already exists", sid) + return errors.Errorf("session %s already exists", sid) } s.mu.sessions[sid] = expiration return nil diff --git a/pkg/sql/txn_state.go b/pkg/sql/txn_state.go index 1b3273c5009a..c0fb8c2c3db8 100644 --- a/pkg/sql/txn_state.go +++ b/pkg/sql/txn_state.go @@ -227,7 +227,7 @@ func (ts *txnState) finishSQLTxn() { ts.cancel = nil } if ts.sp == nil { - panic("No span in context? Was resetForNewSQLTxn() called previously?") + panic(errors.AssertionFailedf("No span in context? Was resetForNewSQLTxn() called previously?")) } if ts.recordingThreshold > 0 { @@ -240,7 +240,7 @@ func (ts *txnState) finishSQLTxn() { } } } else { - log.Warning(ts.Ctx, "Missing trace when sampled was enabled.") + log.Warning(ts.Ctx, "missing trace when sampled was enabled") } } diff --git a/pkg/sqlmigrations/migrations.go b/pkg/sqlmigrations/migrations.go index b3229ef2a3a4..1784710dfd2f 100644 --- a/pkg/sqlmigrations/migrations.go +++ b/pkg/sqlmigrations/migrations.go @@ -857,7 +857,7 @@ func (m *Manager) migrateSystemNamespace( if err != nil { return err } - log.Infof(ctx, "Migrating system.namespace chunk with %d rows", len(rows)) + log.Infof(ctx, "migrating system.namespace chunk with %d rows", len(rows)) for i, row := range rows { workLeft = false // We found some rows from the query, which means that we can't quit @@ -878,7 +878,7 @@ func (m *Manager) migrateSystemNamespace( } // Also create a 'public' schema for this database. schemaKey := sqlbase.NewSchemaKey(id, "public") - log.VEventf(ctx, 2, "Migrating system.namespace entry for database %s", name) + log.VEventf(ctx, 2, "migrating system.namespace entry for database %s", name) if err := txn.Put(ctx, schemaKey.Key(r.codec), keys.PublicSchemaID); err != nil { return err } @@ -892,7 +892,7 @@ func (m *Manager) migrateSystemNamespace( continue } tableKey := sqlbase.NewTableKey(parentID, keys.PublicSchemaID, name) - log.VEventf(ctx, 2, "Migrating system.namespace entry for table %s", name) + log.VEventf(ctx, 2, "migrating system.namespace entry for table %s", name) if err := txn.Put(ctx, tableKey.Key(r.codec), id); err != nil { return err } diff --git a/pkg/storage/cloudimpl/nodelocal_storage.go b/pkg/storage/cloudimpl/nodelocal_storage.go index e5a73b32a3e4..4b25c054a14f 100644 --- a/pkg/storage/cloudimpl/nodelocal_storage.go +++ b/pkg/storage/cloudimpl/nodelocal_storage.go @@ -57,7 +57,7 @@ func makeLocalStorage( ioConf base.ExternalIODirConfig, ) (cloud.ExternalStorage, error) { if cfg.Path == "" { - return nil, errors.Errorf("Local storage requested but path not provided") + return nil, errors.Errorf("local storage requested but path not provided") } client, err := blobClientFactory(ctx, cfg.NodeID) if err != nil { diff --git a/pkg/util/log/doc.go b/pkg/util/log/doc.go index cfd103ed999e..213b77364840 100644 --- a/pkg/util/log/doc.go +++ b/pkg/util/log/doc.go @@ -35,8 +35,8 @@ // // Examples: // -// log.Info(ctx, "Prepare to repel boarders") -// log.Fatal(ctx, "Initialization failed", err) +// log.Info(ctx, "prepare to repel boarders") +// log.Fatal(ctx, "initialization failed", err) // log.Infof(ctx, "client error: %s", err) // // V-Style @@ -49,7 +49,7 @@ // Examples: // // if log.V(2) { -// log.Info(ctx, "Starting transaction...") +// log.Info(ctx, "starting transaction...") // } // // Events diff --git a/pkg/util/metric/registry.go b/pkg/util/metric/registry.go index 28d41da364a7..9cec2021c1e0 100644 --- a/pkg/util/metric/registry.go +++ b/pkg/util/metric/registry.go @@ -71,7 +71,7 @@ func (r *Registry) AddMetric(metric Iterable) { defer r.Unlock() r.tracked = append(r.tracked, metric) if log.V(2) { - log.Infof(context.TODO(), "Added metric: %s (%T)", metric.GetName(), metric) + log.Infof(context.TODO(), "added metric: %s (%T)", metric.GetName(), metric) } } @@ -90,7 +90,7 @@ func (r *Registry) AddMetricStruct(metricStruct interface{}) { tname := tfield.Name if !vfield.CanInterface() { if log.V(2) { - log.Infof(ctx, "Skipping unexported field %s", tname) + log.Infof(ctx, "skipping unexported field %s", tname) } continue } @@ -119,10 +119,10 @@ func (r *Registry) addMetricValue( if val.Kind() == reflect.Ptr && val.IsNil() { if skipNil { if log.V(2) { - log.Infof(ctx, "Skipping nil metric field %s", name) + log.Infof(ctx, "skipping nil metric field %s", name) } } else { - log.Fatalf(ctx, "Found nil metric field %s", name) + log.Fatalf(ctx, "found nil metric field %s", name) } return } @@ -131,7 +131,7 @@ func (r *Registry) addMetricValue( r.AddMetricStruct(typ) default: if log.V(2) { - log.Infof(ctx, "Skipping non-metric field %s", name) + log.Infof(ctx, "skipping non-metric field %s", name) } } } diff --git a/pkg/util/randutil/rand.go b/pkg/util/randutil/rand.go index b0facec10ded..295ad16e8c5f 100644 --- a/pkg/util/randutil/rand.go +++ b/pkg/util/randutil/rand.go @@ -82,5 +82,5 @@ func ReadTestdataBytes(r *rand.Rand, arr []byte) { func SeedForTests() { seed := envutil.EnvOrDefaultInt64("COCKROACH_RANDOM_SEED", NewPseudoSeed()) rand.Seed(seed) - log.Printf("Random seed: %v", seed) + log.Printf("random seed: %v", seed) } From fa29a6d2d2feab0f094dfdd1981a87ab5c90b044 Mon Sep 17 00:00:00 2001 From: Matt Jibson Date: Thu, 20 Aug 2020 12:44:24 -0600 Subject: [PATCH 2/2] opt: teach buildScan how to use inverted histograms Teach stats builder how to detect statistics on inverted index virtual columns that are already correctly being used by buildScan. invertedConstrainScan can then filter the histograms by the inverted constraint. Fixes #48220 Release note: None --- pkg/sql/opt/memo/expr_format.go | 4 + pkg/sql/opt/memo/statistics_builder.go | 131 ++++- pkg/sql/opt/memo/testdata/stats/inverted-geo | 531 +++++++++++++++++++ pkg/sql/opt/props/histogram.go | 78 ++- pkg/sql/opt/xform/memo_format.go | 3 + 5 files changed, 714 insertions(+), 33 deletions(-) create mode 100644 pkg/sql/opt/memo/testdata/stats/inverted-geo diff --git a/pkg/sql/opt/memo/expr_format.go b/pkg/sql/opt/memo/expr_format.go index 8bdd6e022e33..af40dac7fb58 100644 --- a/pkg/sql/opt/memo/expr_format.go +++ b/pkg/sql/opt/memo/expr_format.go @@ -1255,6 +1255,10 @@ func FormatPrivate(f *ExprFmtCtx, private interface{}, physProps *physical.Requi tab := f.Memo.metadata.Table(t.Table) fmt.Fprintf(f.Buffer, " %s", tab.Name()) + case *InvertedFilterPrivate: + col := f.Memo.metadata.ColumnMeta(t.InvertedColumn) + fmt.Fprintf(f.Buffer, " %s", col.Alias) + case *LookupJoinPrivate: tab := f.Memo.metadata.Table(t.Table) if t.Index == cat.PrimaryIndex { diff --git a/pkg/sql/opt/memo/statistics_builder.go b/pkg/sql/opt/memo/statistics_builder.go index 6214795443eb..e19c9a1f981f 100644 --- a/pkg/sql/opt/memo/statistics_builder.go +++ b/pkg/sql/opt/memo/statistics_builder.go @@ -505,8 +505,27 @@ func (sb *statisticsBuilder) makeTableStatistics(tabID opt.TableID) *props.Stati return stats } - // Make now and annotate the metadata table with it for next time. tab := sb.md.Table(tabID) + + // Create a mapping from table column ordinals to inverted index virtual column + // ordinals. This allows us to do a fast lookup while iterating over all stats + // from a statistic's column to any associated virtual columns. We don't merely + // loop over all table columns looking for virtual columns here because other + // things may one day use virtual columns and we want these to be explicitly + // tied to inverted indexes. + invIndexVirtualCols := make(map[int][]int) + for indexI, indexN := 0, tab.IndexCount(); indexI < indexN; indexI++ { + index := tab.Index(indexI) + if !index.IsInverted() { + continue + } + // The first column of inverted indexes is always virtual. + col := index.Column(0) + srcOrd := col.InvertedSourceColumnOrdinal() + invIndexVirtualCols[srcOrd] = append(invIndexVirtualCols[srcOrd], col.Ordinal()) + } + + // Make now and annotate the metadata table with it for next time. stats = &props.Statistics{} if tab.StatisticCount() == 0 { // No statistics. @@ -541,8 +560,28 @@ func (sb *statisticsBuilder) makeTableStatistics(tabID opt.TableID) *props.Stati if cols.Len() == 1 && stat.Histogram() != nil && sb.evalCtx.SessionData.OptimizerUseHistograms { col, _ := cols.Next(0) - colStat.Histogram = &props.Histogram{} - colStat.Histogram.Init(sb.evalCtx, col, stat.Histogram()) + + // If this column is invertable, the histogram describes the inverted index + // entries, and we need to create a new stat for it, and not apply a histogram + // to the source column. + virtualColOrds := invIndexVirtualCols[stat.ColumnOrdinal(0)] + if len(virtualColOrds) == 0 { + colStat.Histogram = &props.Histogram{} + colStat.Histogram.Init(sb.evalCtx, col, stat.Histogram()) + } else { + for _, virtualColOrd := range virtualColOrds { + invCol := tabID.ColumnID(virtualColOrd) + invCols := opt.MakeColSet(invCol) + if invColStat, ok := stats.ColStats.Add(invCols); ok { + invColStat.Histogram = &props.Histogram{} + invColStat.Histogram.Init(sb.evalCtx, invCol, stat.Histogram()) + // Set inverted entry counts from the histogram. + invColStat.DistinctCount = invColStat.Histogram.DistinctValuesCount() + // Inverted indexes don't have nulls. + invColStat.NullCount = 0 + } + } + } } // Make sure the distinct count is at least 1, for the same reason as @@ -616,8 +655,13 @@ func (sb *statisticsBuilder) buildScan(scan *ScanExpr, relProps *props.Relationa // If the constraint has a single span or is an inverted constraint, apply // the constraint selectivity and the partial index predicate (if it exists) // to the underlying table stats. - if scan.InvertedConstraint != nil || - (scan.Constraint != nil && scan.Constraint.Spans.Count() < 2) { + if scan.InvertedConstraint != nil { + // TODO(mjibson): support partial index predicates for inverted constraints. + sb.invertedConstrainScan(scan, relProps) + sb.finalizeFromCardinality(relProps) + return + } + if scan.Constraint != nil && scan.Constraint.Spans.Count() < 2 { sb.constrainScan(scan, scan.Constraint, pred, relProps, s) sb.finalizeFromCardinality(relProps) return @@ -694,11 +738,7 @@ func (sb *statisticsBuilder) constrainScan( // For now, don't apply constraints on inverted index columns. if sb.md.Table(scan.Table).Index(scan.Index).IsInverted() { if scan.InvertedConstraint != nil { - // For now, just assume a single closed span such as ["\xfd", "\xfe"). - // This corresponds to two "conjuncts" as defined in - // numConjunctsInConstraint. - // TODO(rytaft): Use the constraint to estimate selectivity. - numUnappliedConjuncts += 2 + panic(errors.AssertionFailedf("scan.InvertedConstraint not nil")) } if constraint != nil { for i, n := 0, constraint.ConstrainedColumns(sb.evalCtx); i < n; i++ { @@ -747,6 +787,63 @@ func (sb *statisticsBuilder) constrainScan( s.ApplySelectivity(1.0 / sb.selectivityFromSingleColDistinctCounts(histCols, scan, s)) } +// invertedConstrainScan is called from buildScan to calculate the stats for +// the scan based on the given inverted constraint. +func (sb *statisticsBuilder) invertedConstrainScan(scan *ScanExpr, relProps *props.Relational) { + s := &relProps.Stats + + // Calculate distinct counts and histograms for constrained columns + // ---------------------------------------------------------------- + var numUnappliedConjuncts float64 + var constrainedCols, histCols opt.ColSet + idx := sb.md.Table(scan.Table).Index(scan.Index) + // The constrained column is the first (and only) column in the inverted + // index. Using scan.Cols here would also include the PK, which we don't want. + constrainedCols.Add(scan.Table.ColumnID(idx.Column(0).Ordinal())) + if sb.shouldUseHistogram(relProps, constrainedCols) { + constrainedCols.ForEach(func(col opt.ColumnID) { + colSet := opt.MakeColSet(col) + // TODO(mjibson): set distinctCount to something correct. Max is fine for now + // because ensureColStat takes the minimum of the passed value and colSet's + // distinct count. + const distinctCount = math.MaxFloat64 + sb.ensureColStat(colSet, distinctCount, scan, s) + + inputStat, _ := sb.colStatFromInput(colSet, scan) + if inputHist := inputStat.Histogram; inputHist != nil { + // If we have a histogram, set the row count to its total, unfiltered + // count. This is needed because s.RowCount is currently the row count of the + // table, but should instead reflect the number of inverted index entries. + s.RowCount = inputHist.ValuesCount() + if colStat, ok := s.ColStats.Lookup(colSet); ok { + colStat.Histogram = inputHist.InvertedFilter(scan.InvertedConstraint) + histCols.Add(col) + sb.updateDistinctCountFromHistogram(colStat, inputStat.DistinctCount) + } + } else { + // Just assume a single closed span such as ["\xfd", "\xfe"). This corresponds + // to two "conjuncts" as defined in numConjunctsInConstraint. + numUnappliedConjuncts += 2 + } + }) + } else { + // Assume a single closed span. + numUnappliedConjuncts += 2 + } + + // Inverted indexes don't contain NULLs, so we do not need to have + // updateNullCountsFromNotNullCols or selectivityFromNullsRemoved here. + + // Calculate row count and selectivity + // ----------------------------------- + s.ApplySelectivity(sb.selectivityFromHistograms(histCols, scan, s)) + s.ApplySelectivity(sb.selectivityFromMultiColDistinctCounts(constrainedCols, scan, s)) + s.ApplySelectivity(sb.selectivityFromUnappliedConjuncts(numUnappliedConjuncts)) + + // Adjust the selectivity so we don't double-count the histogram columns. + s.ApplySelectivity(1.0 / sb.selectivityFromSingleColDistinctCounts(histCols, scan, s)) +} + func (sb *statisticsBuilder) colStatScan(colSet opt.ColSet, scan *ScanExpr) *props.ColumnStatistic { relProps := scan.Relational() s := &relProps.Stats @@ -2506,14 +2603,20 @@ func (sb *statisticsBuilder) shouldUseHistogram(relProps *props.Relational, cols if relProps.Cardinality.Max < minCardinalityForHistogram { return false } - hasInv := false + allowHist := true cols.ForEach(func(col opt.ColumnID) { colTyp := sb.md.ColumnMeta(col).Type - if sqlbase.ColumnTypeIsInvertedIndexable(colTyp) { - hasInv = true + switch colTyp { + case types.Geometry, types.Geography: + // Special case these since ColumnTypeIsInvertedIndexable returns true for + // them, but they are supported in histograms now. + default: + if sqlbase.ColumnTypeIsInvertedIndexable(colTyp) { + allowHist = false + } } }) - return !hasInv + return allowHist } // rowsProcessed calculates and returns the number of rows processed by the diff --git a/pkg/sql/opt/memo/testdata/stats/inverted-geo b/pkg/sql/opt/memo/testdata/stats/inverted-geo new file mode 100644 index 000000000000..fce192ef1813 --- /dev/null +++ b/pkg/sql/opt/memo/testdata/stats/inverted-geo @@ -0,0 +1,531 @@ +exec-ddl +CREATE TABLE t (i int, g GEOMETRY, INVERTED INDEX (g)) +---- + +# Histogram boundaries are from a `POLYGON((0.0 0.0, 1.0 0.0, 1.0 1.0, +# 0.0 1.0, 0.0 0.0))` row. The row_count is lower than the sum of the +# histogram's num_eq and num_range because there are more entries in +# the inverted index than rows in the table. +exec-ddl +ALTER TABLE t INJECT STATISTICS '[ + { + "columns": ["i"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 2000, + "distinct_count": 1000, + "null_count": 0 + }, + { + "columns": ["g"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 2000, + "distinct_count": 7, + "null_count": 0, + "histo_col_type":"BYTES", + "histo_buckets":[{ + "num_eq":1000, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd0555555555555555" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd0fffffff00000000" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd1000000100000000" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd1aaaaaab00000000" + }] + } +]' +---- + +# Selecting from within the polygon means the histogram will estimate +# many rows returned, thus making a search on the PK favorable. +opt +SELECT i FROM t WHERE st_intersects('LINESTRING(0.5 0.5, 0.7 0.7)', g) ORDER BY i LIMIT 1 +---- +project + ├── columns: i:1(int) + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1] + ├── key: () + ├── fd: ()-->(1) + └── limit + ├── columns: i:1(int) g:2(geometry) + ├── internal-ordering: +1 + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1] + ├── key: () + ├── fd: ()-->(1,2) + ├── sort + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667] + │ ├── ordering: +1 + │ ├── limit hint: 1.00 + │ └── select + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667] + │ ├── scan t + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ └── stats: [rows=2000] + │ └── filters + │ └── st_intersects('010200000002000000000000000000E03F000000000000E03F666666666666E63F666666666666E63F', g:2) [type=bool, outer=(2), immutable] + └── 1 [type=int] + +memo +SELECT i FROM t WHERE st_intersects('LINESTRING(0.5 0.5, 0.7 0.7)', g) ORDER BY i LIMIT 1 +---- +memo (optimized, ~11KB, required=[presentation: i:1]) + ├── G1: (project G2 G3 i) + │ └── [presentation: i:1] + │ ├── best: (project G2 G3 i) + │ └── cost: 4258.50 + ├── G2: (limit G4 G5 ordering=+1) + │ └── [] + │ ├── best: (limit G4="[ordering: +1] [limit hint: 1.00]" G5 ordering=+1) + │ └── cost: 4258.48 + ├── G3: (projections) + ├── G4: (select G6 G7) (select G8 G7) + │ ├── [ordering: +1] [limit hint: 1.00] + │ │ ├── best: (sort G4) + │ │ └── cost: 4258.46 + │ └── [] + │ ├── best: (select G6 G7) + │ └── cost: 4120.04 + ├── G5: (const 1) + ├── G6: (scan t,cols=(1,2)) + │ ├── [ordering: +1] [limit hint: 3.00] + │ │ ├── best: (sort G6) + │ │ └── cost: 2578.66 + │ └── [] + │ ├── best: (scan t,cols=(1,2)) + │ └── cost: 2100.02 + ├── G7: (filters G9) + ├── G8: (index-join G10 t,cols=(1,2)) + │ ├── [ordering: +1] [limit hint: 4.50] + │ │ ├── best: (sort G8) + │ │ └── cost: 16083.08 + │ └── [] + │ ├── best: (index-join G10 t,cols=(1,2)) + │ └── cost: 15330.03 + ├── G9: (function G11 st_intersects) + ├── G10: (inverted-filter G12 g_inverted_key) + │ └── [] + │ ├── best: (inverted-filter G12 g_inverted_key) + │ └── cost: 3150.02 + ├── G11: (scalar-list G13 G14) + ├── G12: (scan t@secondary,cols=(3,5),constrained inverted) + │ └── [] + │ ├── best: (scan t@secondary,cols=(3,5),constrained inverted) + │ └── cost: 3120.01 + ├── G13: (const '010200000002000000000000000000E03F000000000000E03F666666666666E63F666666666666E63F') + └── G14: (variable g) + +# Selecting from outside the polygon means the histogram will estimate +# few rows returned, thus making a search of the inverted index favorable. +opt +SELECT i FROM t WHERE st_intersects('LINESTRING(100 100, 150 150)', g) ORDER BY i LIMIT 1 +---- +project + ├── columns: i:1(int) + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1] + ├── key: () + ├── fd: ()-->(1) + └── limit + ├── columns: i:1(int) g:2(geometry) + ├── internal-ordering: +1 + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1] + ├── key: () + ├── fd: ()-->(1,2) + ├── select + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667] + │ ├── ordering: +1 + │ ├── limit hint: 1.00 + │ ├── sort + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ ├── stats: [rows=7e-07] + │ │ ├── ordering: +1 + │ │ ├── limit hint: 0.00 + │ │ └── index-join t + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ ├── stats: [rows=7e-07] + │ │ └── inverted-filter + │ │ ├── columns: rowid:3(int!null) + │ │ ├── inverted expression: /5 + │ │ │ ├── tight: false + │ │ │ └── union spans: ["\x87\xff", "\x87\xff"] + │ │ ├── stats: [rows=7e-07] + │ │ ├── key: (3) + │ │ └── scan t@secondary + │ │ ├── columns: rowid:3(int!null) g_inverted_key:5(geometry!null) + │ │ ├── inverted constraint: /5/3 + │ │ │ └── spans: ["\x87\xff", "\x87\xff"] + │ │ ├── stats: [rows=7e-07, distinct(3)=1.99999931e-07, null(3)=0, distinct(5)=7e-07, null(5)=0] + │ │ │ histogram(5)= + │ │ ├── key: (3) + │ │ └── fd: (3)-->(5) + │ └── filters + │ └── st_intersects('010200000002000000000000000000594000000000000059400000000000C062400000000000C06240', g:2) [type=bool, outer=(2), immutable] + └── 1 [type=int] + +memo +SELECT i FROM t WHERE st_intersects('LINESTRING(100 100, 150 150)', g) ORDER BY i LIMIT 1 +---- +memo (optimized, ~11KB, required=[presentation: i:1]) + ├── G1: (project G2 G3 i) + │ └── [presentation: i:1] + │ ├── best: (project G2 G3 i) + │ └── cost: 0.10 + ├── G2: (limit G4 G5 ordering=+1) + │ └── [] + │ ├── best: (limit G4="[ordering: +1] [limit hint: 1.00]" G5 ordering=+1) + │ └── cost: 0.08 + ├── G3: (projections) + ├── G4: (select G6 G7) (select G8 G7) + │ ├── [ordering: +1] [limit hint: 1.00] + │ │ ├── best: (select G8="[ordering: +1] [limit hint: 0.00]" G7) + │ │ └── cost: 0.06 + │ └── [] + │ ├── best: (select G8 G7) + │ └── cost: 0.05 + ├── G5: (const 1) + ├── G6: (scan t,cols=(1,2)) + │ ├── [ordering: +1] [limit hint: 3.00] + │ │ ├── best: (sort G6) + │ │ └── cost: 2578.66 + │ └── [] + │ ├── best: (scan t,cols=(1,2)) + │ └── cost: 2100.02 + ├── G7: (filters G9) + ├── G8: (index-join G10 t,cols=(1,2)) + │ ├── [ordering: +1] [limit hint: 0.00] + │ │ ├── best: (sort G8) + │ │ └── cost: 0.04 + │ └── [] + │ ├── best: (index-join G10 t,cols=(1,2)) + │ └── cost: 0.03 + ├── G9: (function G11 st_intersects) + ├── G10: (inverted-filter G12 g_inverted_key) + │ └── [] + │ ├── best: (inverted-filter G12 g_inverted_key) + │ └── cost: 0.02 + ├── G11: (scalar-list G13 G14) + ├── G12: (scan t@secondary,cols=(3,5),constrained inverted) + │ └── [] + │ ├── best: (scan t@secondary,cols=(3,5),constrained inverted) + │ └── cost: 0.01 + ├── G13: (const '010200000002000000000000000000594000000000000059400000000000C062400000000000C06240') + └── G14: (variable g) + +# Add some NULL rows. +exec-ddl +ALTER TABLE t INJECT STATISTICS '[ + { + "columns": ["i"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 2000, + "distinct_count": 1000, + "null_count": 50 + }, + { + "columns": ["g"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 2000, + "distinct_count": 7, + "null_count": 100, + "histo_col_type":"BYTES", + "histo_buckets":[{ + "num_eq":1000, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd0555555555555555" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd0fffffff00000000" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd1000000100000000" + }, + { + "num_eq":1000, + "num_range":1000, + "distinct_range":1, + "upper_bound":"\\xfd1aaaaaab00000000" + }] + } +]' +---- + +# Inverted indexes don't contain NULL entries, so we expect a full scan. +opt colstat=1 colstat=2 +SELECT * FROM t WHERE g IS NULL OR st_intersects('LINESTRING(100 100, 150 150)', g) +---- +select + ├── columns: i:1(int) g:2(geometry) + ├── immutable + ├── stats: [rows=666.666667, distinct(1)=555.555556, null(1)=16.6666667, distinct(2)=7, null(2)=33.3333333] + ├── scan t + │ ├── columns: i:1(int) g:2(geometry) + │ └── stats: [rows=2000, distinct(1)=1000, null(1)=50, distinct(2)=7, null(2)=100] + └── filters + └── (g:2 IS NULL) OR st_intersects('010200000002000000000000000000594000000000000059400000000000C062400000000000C06240', g:2) [type=bool, outer=(2), immutable] + +memo +SELECT * FROM t WHERE g IS NULL OR st_intersects('LINESTRING(100 100, 150 150)', g) +---- +memo (optimized, ~4KB, required=[presentation: i:1,g:2]) + ├── G1: (select G2 G3) + │ └── [presentation: i:1,g:2] + │ ├── best: (select G2 G3) + │ └── cost: 2120.04 + ├── G2: (scan t,cols=(1,2)) + │ └── [] + │ ├── best: (scan t,cols=(1,2)) + │ └── cost: 2100.02 + ├── G3: (filters G4) + ├── G4: (or G5 G6) + ├── G5: (is G7 G8) + ├── G6: (function G9 st_intersects) + ├── G7: (variable g) + ├── G8: (null) + ├── G9: (scalar-list G10 G7) + └── G10: (const '010200000002000000000000000000594000000000000059400000000000C062400000000000C06240') + +# Repeat above tests to ensure null counts are correct. +# TODO(mjibson): Teach logical_props_builder that st_intersects is +# a null-rejecting filter. Since we don't have that logic yet these +# non-zero null counts are correct. +opt colstat=1 colstat=2 +SELECT i FROM t WHERE st_intersects('LINESTRING(0.5 0.5, 0.7 0.7)', g) ORDER BY i LIMIT 1 +---- +project + ├── columns: i:1(int) + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1, distinct(1)=0.99984994, null(1)=0.025, distinct(2)=0.932505476, null(2)=0.05] + ├── key: () + ├── fd: ()-->(1) + └── limit + ├── columns: i:1(int) g:2(geometry) + ├── internal-ordering: +1 + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1, distinct(1)=0.99984994, null(1)=0.025, distinct(2)=0.932505476, null(2)=0.05] + ├── key: () + ├── fd: ()-->(1,2) + ├── sort + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667, distinct(1)=555.555556, null(1)=16.6666667, distinct(2)=7, null(2)=33.3333333] + │ ├── ordering: +1 + │ ├── limit hint: 1.00 + │ └── select + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667, distinct(1)=555.555556, null(1)=16.6666667, distinct(2)=7, null(2)=33.3333333] + │ ├── scan t + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ └── stats: [rows=2000, distinct(1)=1000, null(1)=50, distinct(2)=7, null(2)=100] + │ └── filters + │ └── st_intersects('010200000002000000000000000000E03F000000000000E03F666666666666E63F666666666666E63F', g:2) [type=bool, outer=(2), immutable] + └── 1 [type=int] + + +opt colstat=1 colstat=2 +SELECT i FROM t WHERE st_intersects('LINESTRING(100 100, 150 150)', g) ORDER BY i LIMIT 1 +---- +project + ├── columns: i:1(int) + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1, distinct(1)=0.99984994, null(1)=0.025, distinct(2)=0.932505476, null(2)=0.05] + ├── key: () + ├── fd: ()-->(1) + └── limit + ├── columns: i:1(int) g:2(geometry) + ├── internal-ordering: +1 + ├── cardinality: [0 - 1] + ├── immutable + ├── stats: [rows=1, distinct(1)=0.99984994, null(1)=0.025, distinct(2)=0.932505476, null(2)=0.05] + ├── key: () + ├── fd: ()-->(1,2) + ├── select + │ ├── columns: i:1(int) g:2(geometry) + │ ├── immutable + │ ├── stats: [rows=666.666667, distinct(1)=555.555556, null(1)=16.6666667, distinct(2)=7, null(2)=33.3333333] + │ ├── ordering: +1 + │ ├── limit hint: 1.00 + │ ├── sort + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ ├── stats: [rows=7e-07] + │ │ ├── ordering: +1 + │ │ ├── limit hint: 0.00 + │ │ └── index-join t + │ │ ├── columns: i:1(int) g:2(geometry) + │ │ ├── stats: [rows=7e-07] + │ │ └── inverted-filter + │ │ ├── columns: rowid:3(int!null) + │ │ ├── inverted expression: /5 + │ │ │ ├── tight: false + │ │ │ └── union spans: ["\x87\xff", "\x87\xff"] + │ │ ├── stats: [rows=7e-07] + │ │ ├── key: (3) + │ │ └── scan t@secondary + │ │ ├── columns: rowid:3(int!null) g_inverted_key:5(geometry!null) + │ │ ├── inverted constraint: /5/3 + │ │ │ └── spans: ["\x87\xff", "\x87\xff"] + │ │ ├── stats: [rows=7e-07, distinct(3)=1.99999931e-07, null(3)=0, distinct(5)=7e-07, null(5)=0] + │ │ │ histogram(5)= + │ │ ├── key: (3) + │ │ └── fd: (3)-->(5) + │ └── filters + │ └── st_intersects('010200000002000000000000000000594000000000000059400000000000C062400000000000C06240', g:2) [type=bool, outer=(2), immutable] + └── 1 [type=int] + +# Set a high null count. +exec-ddl +ALTER TABLE t INJECT STATISTICS '[ + { + "columns": ["i"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 1000, + "distinct_count": 100, + "null_count": 900 + }, + { + "columns": ["g"], + "created_at": "2018-01-01 1:00:00.00000+00:00", + "row_count": 1000, + "distinct_count": 4, + "null_count": 900, + "histo_col_type":"BYTES", + "histo_buckets":[{ + "num_eq":100, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd0555555555555555" + }, + { + "num_eq":100, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd0fffffff00000000" + }, + { + "num_eq":100, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd1000000100000000" + }, + { + "num_eq":100, + "num_range":0, + "distinct_range":0, + "upper_bound":"\\xfd1aaaaaab00000000" + }] + } +]' +---- + +opt colstat=1 colstat=2 +SELECT * FROM t WHERE st_intersects('LINESTRING(.5 .5, .7 .7)', g) +---- +select + ├── columns: i:1(int) g:2(geometry) + ├── immutable + ├── stats: [rows=333.333333, distinct(1)=98.265847, null(1)=300, distinct(2)=4, null(2)=300] + ├── index-join t + │ ├── columns: i:1(int) g:2(geometry) + │ ├── stats: [rows=100] + │ └── inverted-filter + │ ├── columns: rowid:3(int!null) + │ ├── inverted expression: /5 + │ │ ├── tight: false + │ │ └── union spans + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x00", "\xfd\x10\x00\x00\x00\x00\x00\x00\x00"] + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x01", "\xfd\x12") + │ │ └── ["\xfd\x14\x00\x00\x00\x00\x00\x00\x00", "\xfd\x14\x00\x00\x00\x00\x00\x00\x00"] + │ ├── stats: [rows=100] + │ ├── key: (3) + │ └── scan t@secondary + │ ├── columns: rowid:3(int!null) g_inverted_key:5(geometry!null) + │ ├── inverted constraint: /5/3 + │ │ └── spans + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x00", "\xfd\x10\x00\x00\x00\x00\x00\x00\x00"] + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x01", "\xfd\x12") + │ │ └── ["\xfd\x14\x00\x00\x00\x00\x00\x00\x00", "\xfd\x14\x00\x00\x00\x00\x00\x00\x00"] + │ ├── stats: [rows=100, distinct(3)=100, null(3)=0, distinct(5)=1, null(5)=0] + │ │ histogram(5)= 0 100 0 0 + │ │ <--- '\xfd1000000100000000' --- '\xfd1400000000000001' + │ ├── key: (3) + │ └── fd: (3)-->(5) + └── filters + └── st_intersects('010200000002000000000000000000E03F000000000000E03F666666666666E63F666666666666E63F', g:2) [type=bool, outer=(2), immutable] + +# Force a scan of the inverted index so we can see the filtered histogram. +opt +SELECT i FROM t@secondary WHERE st_intersects('LINESTRING(.5 .5, .7 .7)', g) +---- +project + ├── columns: i:1(int) + ├── immutable + ├── stats: [rows=333.333333] + └── select + ├── columns: i:1(int) g:2(geometry) + ├── immutable + ├── stats: [rows=333.333333] + ├── index-join t + │ ├── columns: i:1(int) g:2(geometry) + │ ├── stats: [rows=100] + │ └── inverted-filter + │ ├── columns: rowid:3(int!null) + │ ├── inverted expression: /5 + │ │ ├── tight: false + │ │ └── union spans + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x00", "\xfd\x10\x00\x00\x00\x00\x00\x00\x00"] + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x01", "\xfd\x12") + │ │ └── ["\xfd\x14\x00\x00\x00\x00\x00\x00\x00", "\xfd\x14\x00\x00\x00\x00\x00\x00\x00"] + │ ├── stats: [rows=100] + │ ├── key: (3) + │ └── scan t@secondary + │ ├── columns: rowid:3(int!null) g_inverted_key:5(geometry!null) + │ ├── inverted constraint: /5/3 + │ │ └── spans + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x00", "\xfd\x10\x00\x00\x00\x00\x00\x00\x00"] + │ │ ├── ["\xfd\x10\x00\x00\x00\x00\x00\x00\x01", "\xfd\x12") + │ │ └── ["\xfd\x14\x00\x00\x00\x00\x00\x00\x00", "\xfd\x14\x00\x00\x00\x00\x00\x00\x00"] + │ ├── flags: force-index=secondary + │ ├── stats: [rows=100, distinct(3)=100, null(3)=0, distinct(5)=1, null(5)=0] + │ │ histogram(5)= 0 100 0 0 + │ │ <--- '\xfd1000000100000000' --- '\xfd1400000000000001' + │ ├── key: (3) + │ └── fd: (3)-->(5) + └── filters + └── st_intersects('010200000002000000000000000000E03F000000000000E03F666666666666E63F666666666666E63F', g:2) [type=bool, outer=(2), immutable] diff --git a/pkg/sql/opt/props/histogram.go b/pkg/sql/opt/props/histogram.go index 8a66d75ead69..630a3446778e 100644 --- a/pkg/sql/opt/props/histogram.go +++ b/pkg/sql/opt/props/histogram.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/cat" "github.com/cockroachdb/cockroach/pkg/sql/opt/constraint" + "github.com/cockroachdb/cockroach/pkg/sql/opt/invertedexpr" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/errors" @@ -178,15 +179,14 @@ func (h *Histogram) CanFilter(c *constraint.Constraint) (colOffset, exactPrefix return 0, exactPrefix, false } -// Filter filters the histogram according to the given constraint, and returns -// a new histogram with the results. CanFilter should be called first to -// validate that c can filter the histogram. -func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { - colOffset, exactPrefix, ok := h.CanFilter(c) - if !ok { - panic(errors.AssertionFailedf("column mismatch")) - } - +func (h *Histogram) filter( + spanCount int, + getSpan func(int) *constraint.Span, + desc bool, + colOffset, exactPrefix int, + prefix []tree.Datum, + columns constraint.Columns, +) *Histogram { bucketCount := h.BucketCount() filtered := &Histogram{ evalCtx: h.evalCtx, @@ -203,21 +203,15 @@ func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { panic(errors.AssertionFailedf("the first bucket should have NumRange=0")) } - prefix := make([]tree.Datum, colOffset) - for i := range prefix { - prefix[i] = c.Spans.Get(0).StartKey().Value(i) - } - desc := c.Columns.Get(colOffset).Descending() var iter histogramIter iter.init(h, desc) spanIndex := 0 - keyCtx := constraint.KeyContext{EvalCtx: h.evalCtx, Columns: c.Columns} + keyCtx := constraint.KeyContext{EvalCtx: h.evalCtx, Columns: columns} // Find the first span that may overlap with the histogram. firstBucket := makeSpanFromBucket(&iter, prefix) - spanCount := c.Spans.Count() for spanIndex < spanCount { - span := c.Spans.Get(spanIndex) + span := getSpan(spanIndex) if firstBucket.StartsAfter(&keyCtx, span) { spanIndex++ continue @@ -229,7 +223,7 @@ func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { } // Use binary search to find the first bucket that overlaps with the span. - span := c.Spans.Get(spanIndex) + span := getSpan(spanIndex) bucIndex := sort.Search(bucketCount, func(i int) bool { iter.setIdx(i) bucket := makeSpanFromBucket(&iter, prefix) @@ -263,7 +257,7 @@ func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { // Convert the bucket to a span in order to take advantage of the // constraint library. left := makeSpanFromBucket(&iter, prefix) - right := c.Spans.Get(spanIndex) + right := getSpan(spanIndex) if left.StartsAfter(&keyCtx, right) { spanIndex++ @@ -335,6 +329,52 @@ func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { return filtered } +// Filter filters the histogram according to the given constraint, and returns +// a new histogram with the results. CanFilter should be called first to +// validate that c can filter the histogram. +func (h *Histogram) Filter(c *constraint.Constraint) *Histogram { + colOffset, exactPrefix, ok := h.CanFilter(c) + if !ok { + panic(errors.AssertionFailedf("column mismatch")) + } + prefix := make([]tree.Datum, colOffset) + for i := range prefix { + prefix[i] = c.Spans.Get(0).StartKey().Value(i) + } + desc := c.Columns.Get(colOffset).Descending() + + return h.filter(c.Spans.Count(), c.Spans.Get, desc, colOffset, exactPrefix, prefix, c.Columns) +} + +// InvertedFilter filters the histogram according to the given inverted +// constraint, and returns a new histogram with the results. +func (h *Histogram) InvertedFilter(spans invertedexpr.InvertedSpans) *Histogram { + var columns constraint.Columns + columns.InitSingle(opt.MakeOrderingColumn(h.col, false /* desc */)) + return h.filter( + len(spans), + func(idx int) *constraint.Span { + return makeSpanFromInvertedSpan(spans[idx]) + }, + false, /* desc */ + 0, /* exactPrefix */ + 0, /* colOffset */ + nil, /* prefix */ + columns, + ) +} + +func makeSpanFromInvertedSpan(invSpan invertedexpr.InvertedSpan) *constraint.Span { + var span constraint.Span + span.Init( + constraint.MakeKey(tree.NewDBytes(tree.DBytes(invSpan.Start))), + constraint.IncludeBoundary, + constraint.MakeKey(tree.NewDBytes(tree.DBytes(invSpan.End))), + constraint.ExcludeBoundary, + ) + return &span +} + func (h *Histogram) getNextLowerBound(currentUpperBound tree.Datum) tree.Datum { nextLowerBound, ok := currentUpperBound.Next(h.evalCtx) if !ok { diff --git a/pkg/sql/opt/xform/memo_format.go b/pkg/sql/opt/xform/memo_format.go index 07cfdafa0d64..64500508d088 100644 --- a/pkg/sql/opt/xform/memo_format.go +++ b/pkg/sql/opt/xform/memo_format.go @@ -281,6 +281,9 @@ func (mf *memoFormatter) formatPrivate(e opt.Expr, physProps *physical.Required) if t.Constraint != nil { fmt.Fprintf(mf.buf, ",constrained") } + if t.InvertedConstraint != nil { + fmt.Fprintf(mf.buf, ",constrained inverted") + } if t.HardLimit.IsSet() { fmt.Fprintf(mf.buf, ",lim=%s", t.HardLimit) }