Merge #67210 #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 <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Jul 13, 2021 · b3b850e · b3b850e
3 parents dd1cdfc + 19ecc2c + e1e9d81
commit b3b850e
Show file tree

Hide file tree

Showing 42 changed files with 851 additions and 523 deletions.
diff --git a/pkg/BUILD.bazel b/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",

diff --git a/pkg/ccl/changefeedccl/BUILD.bazel b/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",

diff --git a/pkg/ccl/changefeedccl/avro.go b/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:

diff --git a/pkg/cli/BUILD.bazel b/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",

diff --git a/pkg/cli/clisqlclient/BUILD.bazel b/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",
+    ],
+)
diff --git a/pkg/cli/clisqlclient/doc.go b/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
diff --git a/pkg/cli/clisqlclient/main_test.go b/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
diff --git a/pkg/cli/sql_format_value.go → pkg/cli/clisqlclient/sql_format_value.go b/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.

diff --git a/pkg/cli/sql_format_value_test.go → ...cli/clisqlclient/sql_format_value_test.go b/pkg/cli/sql_format_value_test.go → ...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)"})

diff --git a/pkg/cli/clisqlshell/BUILD.bazel b/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"],
+)
diff --git a/pkg/cli/clisqlshell/doc.go b/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
diff --git a/pkg/cli/sql.go b/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.

diff --git a/pkg/cli/sql_util.go b/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
 }

diff --git a/pkg/sql/BUILD.bazel b/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",

diff --git a/pkg/sql/conn_executor_test.go b/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 {