Merge cockroachdb#67210 cockroachdb#67456

67210: sql: respect IntervalStyle for parsing r=rafiss a=otan

See individual commits for details.

67456: cli: extract the value formatting code to a sub-package (1/) r=otan a=knz

This is part of a family of PRs that aim to encapsulate CLI functionality into finer grained packages.

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

pkg/BUILD.bazel

@@ -45,6 +45,7 @@ ALL_TESTS = [
  "//pkg/ccl/utilccl:utilccl_test",
  "//pkg/ccl/workloadccl/allccl:allccl_test",
  "//pkg/ccl/workloadccl:workloadccl_test",
+ "//pkg/cli/clisqlclient:clisqlclient_test",
  "//pkg/cli/exit:exit_test",
  "//pkg/cli:cli_test",
  "//pkg/clusterversion:clusterversion_test",

pkg/ccl/changefeedccl/BUILD.bazel

@@ -80,6 +80,7 @@ go_library(
  "//pkg/util",
  "//pkg/util/bitarray",
  "//pkg/util/bufalloc",
+ "//pkg/util/duration",
  "//pkg/util/encoding",
  "//pkg/util/errorutil",
  "//pkg/util/hlc",

pkg/ccl/changefeedccl/avro.go

@@ -21,6 +21,7 @@ import (
  "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
  "github.com/cockroachdb/cockroach/pkg/sql/types"
  "github.com/cockroachdb/cockroach/pkg/util/bitarray"
+ "github.com/cockroachdb/cockroach/pkg/util/duration"
  "github.com/cockroachdb/cockroach/pkg/util/hlc"
  "github.com/cockroachdb/cockroach/pkg/util/timeofday"
  "github.com/cockroachdb/errors"
@@ -493,7 +494,7 @@ func typeToAvroSchema(typ *types.T) (*avroSchemaField, error) {
  return d.(*tree.DInterval).ValueAsISO8601String(), nil
  },
  func(x interface{}) (tree.Datum, error) {
- return tree.ParseDInterval(x.(string))
+ return tree.ParseDInterval(duration.IntervalStyle_ISO_8601, x.(string))
  },
  )
  case types.DecimalFamily:

pkg/cli/BUILD.bazel

@@ -47,7 +47,6 @@ go_library(
  "quit.go",
  "sql.go",
  "sql_format_table.go",
- "sql_format_value.go",
  "sql_util.go",
  "sqlfmt.go",
  "start.go",
@@ -99,6 +98,7 @@ go_library(
  "//pkg/ccl/sqlproxyccl",
  "//pkg/ccl/sqlproxyccl/tenantdirsvr",
  "//pkg/cli/cliflags",
+ "//pkg/cli/clisqlclient",
  "//pkg/cli/exit",
  "//pkg/cli/syncbench",
  "//pkg/clusterversion",
@@ -147,7 +147,6 @@ go_library(
  "//pkg/sql/row",
  "//pkg/sql/sem/builtins",
  "//pkg/sql/sem/tree",
- "//pkg/sql/sessiondatapb",
  "//pkg/sqlmigrations",
  "//pkg/storage",
  "//pkg/storage/cloud",
@@ -160,6 +159,7 @@ go_library(
  "//pkg/util",
  "//pkg/util/cgroups",
  "//pkg/util/contextutil",
+ "//pkg/util/duration",
  "//pkg/util/encoding",
  "//pkg/util/encoding/csv",
  "//pkg/util/envutil",
@@ -302,7 +302,6 @@ go_test(
  "nodelocal_test.go",
  "quit_test.go",
  "sql_format_table_test.go",
- "sql_format_value_test.go",
  "sql_test.go",
  "sql_util_test.go",
  "sqlfmt_test.go",

pkg/cli/clisqlclient/BUILD.bazel

@@ -0,0 +1,32 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+ name = "clisqlclient",
+ srcs = [
+ "doc.go",
+ "sql_format_value.go",
+ ],
+ importpath = "github.com/cockroachdb/cockroach/pkg/cli/clisqlclient",
+ visibility = ["//visibility:public"],
+ deps = [
+ "//pkg/sql/lex",
+ "//pkg/sql/sessiondatapb",
+ "//pkg/util/log",
+ "//pkg/util/timeutil",
+ "@com_github_lib_pq//:pq",
+ ],
+)
+
+go_test(
+ name = "clisqlclient_test",
+ srcs = [
+ "main_test.go",
+ "sql_format_value_test.go",
+ ],
+ deps = [
+ "//pkg/build",
+ "//pkg/cli",
+ "//pkg/server",
+ "//pkg/testutils/serverutils",
+ ],
+)

pkg/cli/clisqlclient/doc.go

@@ -0,0 +1,15 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+// Package clisqlclient contains code common to all CLI commands
+// that establish SQL connections, including but not exclusively
+// the SQL interactive shell. It also supports commands like
+// 'cockroach node ls'.
+package clisqlclient

pkg/cli/clisqlclient/main_test.go

@@ -0,0 +1,31 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package clisqlclient_test
+
+import (
+ "os"
+ "testing"
+
+ "github.com/cockroachdb/cockroach/pkg/build"
+ "github.com/cockroachdb/cockroach/pkg/server"
+ "github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+)
+
+func TestMain(m *testing.M) {
+ // CLI tests are sensitive to the server version, but test binaries don't have
+ // a version injected. Pretend to be a very up-to-date version.
+ defer build.TestingOverrideTag("v999.0.0")()
+
+ serverutils.InitTestServerFactory(server.TestServerFactory)
+ os.Exit(m.Run())
+}
+
+//go:generate ../../util/leaktest/add-leaktest.sh *_test.go

pkg/cli/sql_format_value.go → pkg/cli/clisqlclient/sql_format_value.go

@@ -8,7 +8,7 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
-package cli
+package clisqlclient
 
 import (
  "context"
@@ -37,7 +37,9 @@ func isNotGraphicUnicodeOrTabOrNewline(r rune) bool {
  return r != '\t' && r != '\n' && !unicode.IsGraphic(r)
 }
 
-func formatVal(
+// FormatVal formats a value retrieved by a SQL driver into a string
+// suitable for displaying to the user.
+func FormatVal(
  val driver.Value, colType string, showPrintableUnicode bool, showNewLinesAndTabs bool,
 ) string {
  log.VInfof(context.Background(), 2, "value: go %T, sql %q", val, colType)
@@ -168,7 +170,7 @@ func formatArray(
  // A parsing failure is not a catastrophe; we can still print out
  // the array as a byte slice. This will do in many cases.
  log.VInfof(context.Background(), 1, "unable to parse %q (sql %q) as array: %v", b, colType, err)
- return formatVal(b, "BYTEA", showPrintableUnicode, showNewLinesAndTabs)
+ return FormatVal(b, "BYTEA", showPrintableUnicode, showNewLinesAndTabs)
  }
 
  // We have a go array in "backingArray". Now print it out.
@@ -182,7 +184,7 @@ func formatArray(
  // Access the i-th element in the backingArray.
  arrayVal := driver.Value(v.Index(i).Interface())
  // Format the value recursively into a string.
- vs := formatVal(arrayVal, colType, showPrintableUnicode, showNewLinesAndTabs)
+ vs := FormatVal(arrayVal, colType, showPrintableUnicode, showNewLinesAndTabs)
 
  // If the value contains special characters or a comma, enclose in double quotes.
  // Also escape the special characters.

pkg/cli/sql_format_value_test.go → pkg/cli/clisqlclient/sql_format_value_test.go

@@ -8,10 +8,12 @@
 // by the Apache License, Version 2.0, included in the file
 // licenses/APL.txt.
 
-package cli
+package clisqlclient_test
+
+import "github.com/cockroachdb/cockroach/pkg/cli"
 
 func Example_sql_format() {
- c := NewCLITest(TestCLIParams{})
+ c := cli.NewCLITest(cli.TestCLIParams{})
  defer c.Cleanup()
 
  c.RunWithArgs([]string{"sql", "-e", "create database t; create table t.times (bare timestamp, withtz timestamptz)"})

pkg/cli/clisqlshell/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+ name = "clisqlshell",
+ srcs = ["doc.go"],
+ importpath = "github.com/cockroachdb/cockroach/pkg/cli/clisqlshell",
+ visibility = ["//visibility:public"],
+)

pkg/cli/clisqlshell/doc.go

@@ -0,0 +1,17 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+// Package clisqlshell contains the code that powers CockroachDB's
+// interactive SQL shell.
+//
+// Note that the common code that is shared with other CLI commands
+// that are not interactive SQL shells but establish a SQL connection
+// to a server should be placed in package clisqlclient instead.
+package clisqlshell

pkg/cli/sql.go

@@ -27,6 +27,7 @@ import (
  "time"
 
  "github.com/cockroachdb/cockroach/pkg/cli/cliflags"
+ "github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
  "github.com/cockroachdb/cockroach/pkg/docs"
  "github.com/cockroachdb/cockroach/pkg/roachpb"
  "github.com/cockroachdb/cockroach/pkg/sql"
@@ -848,7 +849,7 @@ func (c *cliState) refreshTransactionStatus() {
  return
  }
 
- txnString := formatVal(dbVal, dbColType,
+ txnString := clisqlclient.FormatVal(dbVal, dbColType,
  false /* showPrintableUnicode */, false /* shownewLinesAndTabs */)
 
  // Change the prompt based on the response from the server.
@@ -887,7 +888,7 @@ func (c *cliState) refreshDatabaseName() string {
  " Use SET database = <dbname> to change, CREATE DATABASE to make a new database.")
  }
 
- dbName := formatVal(dbVal, dbColType,
+ dbName := clisqlclient.FormatVal(dbVal, dbColType,
  false /* showPrintableUnicode */, false /* shownewLinesAndTabs */)
 
  // Preserve the current database name in case of reconnects.

pkg/cli/sql_util.go

@@ -23,11 +23,13 @@ import (
 
  "github.com/cockroachdb/cockroach-go/crdb"
  "github.com/cockroachdb/cockroach/pkg/build"
+ "github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
  "github.com/cockroachdb/cockroach/pkg/roachpb"
  "github.com/cockroachdb/cockroach/pkg/security"
  "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
  "github.com/cockroachdb/cockroach/pkg/sql/parser"
  "github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+ "github.com/cockroachdb/cockroach/pkg/util/duration"
  "github.com/cockroachdb/cockroach/pkg/util/envutil"
  "github.com/cockroachdb/cockroach/pkg/util/log"
  "github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -425,12 +427,12 @@ func (c *sqlConn) getLastQueryStatistics() (
  return 0, 0, 0, 0, 0, containsJobLat, err
  }
 
- parseLatencyRaw = formatVal(row[0], iter.colTypes[0], false, false)
- planLatencyRaw = formatVal(row[1], iter.colTypes[1], false, false)
- execLatencyRaw = formatVal(row[2], iter.colTypes[2], false, false)
- serviceLatencyRaw = formatVal(row[3], iter.colTypes[3], false, false)
+ parseLatencyRaw = clisqlclient.FormatVal(row[0], iter.colTypes[0], false, false)
+ planLatencyRaw = clisqlclient.FormatVal(row[1], iter.colTypes[1], false, false)
+ execLatencyRaw = clisqlclient.FormatVal(row[2], iter.colTypes[2], false, false)
+ serviceLatencyRaw = clisqlclient.FormatVal(row[3], iter.colTypes[3], false, false)
  if containsJobLat {
- jobsLatencyRaw = formatVal(row[4], iter.colTypes[4], false, false)
+ jobsLatencyRaw = clisqlclient.FormatVal(row[4], iter.colTypes[4], false, false)
  }
 
  nRows++
@@ -441,13 +443,16 @@ func (c *sqlConn) getLastQueryStatistics() (
  errors.Newf("unexpected number of rows in SHOW LAST QUERY STATISTICS: %d", nRows)
  }
 
- parsedExecLatency, _ := tree.ParseDInterval(execLatencyRaw)
- parsedServiceLatency, _ := tree.ParseDInterval(serviceLatencyRaw)
- parsedPlanLatency, _ := tree.ParseDInterval(planLatencyRaw)
- parsedParseLatency, _ := tree.ParseDInterval(parseLatencyRaw)
+ // This should really be the same as the session's IntervalStyle
+ // but that only effects negative intervals in the magnitude
+ // of days - and all these latencies should be positive.
+ parsedExecLatency, _ := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, execLatencyRaw)
+ parsedServiceLatency, _ := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, serviceLatencyRaw)
+ parsedPlanLatency, _ := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, planLatencyRaw)
+ parsedParseLatency, _ := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, parseLatencyRaw)
 
  if containsJobLat {
- parsedJobsLatency, _ := tree.ParseDInterval(jobsLatencyRaw)
+ parsedJobsLatency, _ := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, jobsLatencyRaw)
  jobsLat = time.Duration(parsedJobsLatency.Duration.Nanos())
  }
 
@@ -1075,7 +1080,7 @@ func getColumnStrings(rows *sqlRows, showMoreChars bool) []string {
  srcCols := rows.Columns()
  cols := make([]string, len(srcCols))
  for i, c := range srcCols {
- cols[i] = formatVal(c, "NAME", showMoreChars, showMoreChars)
+ cols[i] = clisqlclient.FormatVal(c, "NAME", showMoreChars, showMoreChars)
  }
  return cols
 }
@@ -1114,7 +1119,7 @@ func getNextRowStrings(rows *sqlRows, colTypes []string, showMoreChars bool) ([]
 
  rowStrings := make([]string, len(cols))
  for i, v := range vals {
- rowStrings[i] = formatVal(v, colTypes[i], showMoreChars, showMoreChars)
+ rowStrings[i] = clisqlclient.FormatVal(v, colTypes[i], showMoreChars, showMoreChars)
  }
  return rowStrings, nil
 }

pkg/sql/BUILD.bazel

@@ -597,6 +597,7 @@ go_test(
  "//pkg/util/caller",
  "//pkg/util/cancelchecker",
  "//pkg/util/ctxgroup",
+ "//pkg/util/duration",
  "//pkg/util/encoding",
  "//pkg/util/fsm",
  "//pkg/util/hlc",

pkg/sql/conn_executor_test.go

@@ -44,6 +44,7 @@ import (
  "github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
  "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
  "github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
+ "github.com/cockroachdb/cockroach/pkg/util/duration"
  "github.com/cockroachdb/cockroach/pkg/util/leaktest"
  "github.com/cockroachdb/cockroach/pkg/util/log"
  "github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -1002,19 +1003,19 @@ ALTER TABLE t1 ADD COLUMN b INT DEFAULT 1`,
  )
  require.NoError(t, err, "unexpected error while reading last query statistics")
 
- parseInterval, err := tree.ParseDInterval(parseLatency)
+ parseInterval, err := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, parseLatency)
  require.NoError(t, err)
 
- planInterval, err := tree.ParseDInterval(planLatency)
+ planInterval, err := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, planLatency)
  require.NoError(t, err)
 
- execInterval, err := tree.ParseDInterval(execLatency)
+ execInterval, err := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, execLatency)
  require.NoError(t, err)
 
- serviceInterval, err := tree.ParseDInterval(serviceLatency)
+ serviceInterval, err := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, serviceLatency)
  require.NoError(t, err)
 
- postCommitJobsInterval, err := tree.ParseDInterval(postCommitJobsLatency)
+ postCommitJobsInterval, err := tree.ParseDInterval(duration.IntervalStyle_POSTGRES, postCommitJobsLatency)
  require.NoError(t, err)
 
  if parseInterval.AsFloat64() <= 0 || parseInterval.AsFloat64() > 1 {