Merge #32144

32144: pgwire text encoding fixes r=mjibson a=mjibson Most commits here are bug fixes or test improvements. Arrays/tuples and intervals got significant changes to match postgres (but they weren't broken before), and are benchmarked below. Array encoding delta: ``` name old time/op new time/op delta Encodings/array['']/text-8 511ns ± 8% 475ns ± 4% ~ (p=0.095 n=5+5) Encodings/array['\x0d53e338548082'::BYTEA]/text-8 846ns ±10% 604ns ± 9% -28.57% (p=0.008 n=5+5) Encodings/array['test_with_spaces'::BYTEA]/text-8 883ns ± 3% 719ns ±10% -18.66% (p=0.008 n=5+5) Encodings/array['name'::NAME]/text-8 893ns ± 6% 599ns ±11% -32.95% (p=0.008 n=5+5) Encodings/array[NULL]::text[]/text-8 615ns ± 9% 523ns ± 2% -14.99% (p=0.008 n=5+5) Encodings/array['']::text[]/text-8 587ns ± 1% 553ns ± 9% ~ (p=0.151 n=5+5) Encodings/array['test']::text[]/text-8 651ns ± 3% 579ns ± 8% -10.97% (p=0.008 n=5+5) Encodings/array['test_with_spaces']::text[]/text-8 837ns ± 9% 688ns ± 7% -17.79% (p=0.008 n=5+5) Encodings/array[e'\f']::text[]/text-8 634ns ± 2% 550ns ± 8% -13.27% (p=0.008 n=5+5) Encodings/array[e'\uFEFF']::text[]/text-8 1.18µs ± 4% 0.57µs ± 3% -51.94% (p=0.008 n=5+5) Encodings/array[e'\u2603']::text[]/text-8 799ns ± 3% 573ns ± 7% -28.25% (p=0.008 n=5+5) Encodings/(1::int8,2::int8,3::int8,4::int8,null)/text-8 488ns ± 7% 619ns ± 3% +26.99% (p=0.008 n=5+5) Encodings/('test_with_spaces'::BYTEA,null)/text-8 720ns ±27% 460ns ± 8% -36.12% (p=0.008 n=5+5) Encodings/('test_with_spaces'::TEXT,null)/text-8 517ns ± 9% 437ns ±10% -15.51% (p=0.008 n=5+5) Encodings/('name'::NAME,null)/text-8 525ns ± 6% 325ns ± 7% -38.02% (p=0.008 n=5+5) Encodings/('false'::JSONB,null)/text-8 1.04µs ± 7% 0.63µs ± 6% -39.00% (p=0.008 n=5+5) Encodings/('{"a":_[]}'::JSONB,null)/text-8 1.25µs ± 6% 0.75µs ±13% -40.44% (p=0.008 n=5+5) ``` Interval encoding delta: ``` name old time/op new time/op delta Encodings/'10y10mon'::interval/text-8 836ns ± 4% 1055ns ± 4% +26.19% (p=0.008 n=5+5) Encodings/'1y1mon'::interval/text-8 830ns ± 6% 1019ns ±15% +22.78% (p=0.008 n=5+5) Encodings/'23:12:34'::interval/text-8 1.00µs ± 5% 1.10µs ± 9% +9.18% (p=0.032 n=5+5) Encodings/'-23:00:00'::interval/text-8 791ns ± 7% 1016ns ± 5% +28.56% (p=0.008 n=5+5) Encodings/'-10d'::interval/text-8 798ns ±12% 958ns ± 3% +20.03% (p=0.008 n=5+5) Encodings/'1ms'::interval/text-8 765ns ± 3% 1418ns ± 6% +85.36% (p=0.008 n=5+5) Encodings/'.2ms'::interval/text-8 777ns ± 3% 1387ns ± 4% +78.39% (p=0.008 n=5+5) Encodings/'-1mon1m'::interval/text-8 898ns ± 5% 1386ns ± 9% +54.29% (p=0.008 n=5+5) Encodings/'-1y1m'::interval/text-8 900ns ± 7% 1380ns ± 1% +53.34% (p=0.008 n=5+5) Encodings/'3y4mon5d6ms'::interval/text-8 1.13µs ± 2% 2.23µs ±13% +97.24% (p=0.008 n=5+5) Encodings/'296537y20d15h30m7s'::interval/text-8 1.35µs ± 8% 1.77µs ± 8% +31.03% (p=0.008 n=5+5) Encodings/'-2965y_-20d_-15h_-30m_-7s'::interval/text-8 1.39µs ± 6% 1.67µs ± 4% +19.92% (p=0.008 n=5+5) Encodings/'00:00:00'::interval/text-8 566ns ± 3% 583ns ± 3% ~ (p=0.175 n=5+5) ``` Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
cockroachdb · Nov 27, 2018 · 37eb714 · 37eb714
2 parents f3e6225 + 8c7a60f
commit 37eb714
Show file tree

Hide file tree

Showing 95 changed files with 8,513 additions and 2,185 deletions.
diff --git a/.gitattributes b/.gitattributes
@@ -1 +1,5 @@
 *.pb.* -diff
+
+# Due to bytes not being escaped during array encoding, this file gets
+# rendered as binary.
+pkg/sql/logictest/testdata/logic_test/array diff
diff --git a/pkg/cli/sql_util_test.go b/pkg/cli/sql_util_test.go
@@ -135,8 +135,8 @@ SET
 	}
 
 	expectedRows := [][]string{
-		{`parentID`, `INT`, `false`, `NULL`, ``, `{"primary"}`, `false`},
-		{`name`, `STRING`, `false`, `NULL`, ``, `{"primary"}`, `false`},
+		{`parentID`, `INT`, `false`, `NULL`, ``, `{primary}`, `false`},
+		{`name`, `STRING`, `false`, `NULL`, ``, `{primary}`, `false`},
 		{`id`, `INT`, `true`, `NULL`, ``, `{}`, `false`},
 	}
 	if !reflect.DeepEqual(expectedRows, rows) {
@@ -149,11 +149,11 @@ SET
 	}
 
 	expected = `
-  column_name | data_type | is_nullable | column_default | generation_expression |   indices   | is_hidden  
-+-------------+-----------+-------------+----------------+-----------------------+-------------+-----------+
-  parentID    | INT       |    false    | NULL           |                       | {"primary"} |   false    
-  name        | STRING    |    false    | NULL           |                       | {"primary"} |   false    
-  id          | INT       |    true     | NULL           |                       | {}          |   false    
+  column_name | data_type | is_nullable | column_default | generation_expression |  indices  | is_hidden  
++-------------+-----------+-------------+----------------+-----------------------+-----------+-----------+
+  parentID    | INT       |    false    | NULL           |                       | {primary} |   false    
+  name        | STRING    |    false    | NULL           |                       | {primary} |   false    
+  id          | INT       |    true     | NULL           |                       | {}        |   false    
 (3 rows)
 `
 

diff --git a/pkg/cli/testdata/dump/row b/pkg/cli/testdata/dump/row
@@ -84,11 +84,10 @@ CREATE TABLE t (
 );
 
 INSERT INTO t (i, f, s, b, d, t, ts, n, o, e, u, ip, j, ary, tz, e1, e2, s1) VALUES
-	(1, 2.3, 'striiing', '\x613162326333', '2016-03-26', '01:02:03.456', '2016-01-25 10:10:10+00:00', '2h30m30s', true, 1.2345, 'e9716c74-2638-443d-90ed-ffde7bea7d1d', '192.168.0.1', '{"a": "b"}', ARRAY['hello','world'], '2016-01-25 10:10:10+00:00', 3, 4.5, 's'),
+	(1, 2.3, 'striiing', '\x613162326333', '2016-03-26', '01:02:03.456', '2016-01-25 10:10:10+00:00', '02:30:30', true, 1.2345, 'e9716c74-2638-443d-90ed-ffde7bea7d1d', '192.168.0.1', '{"a": "b"}', ARRAY['hello','world'], '2016-01-25 10:10:10+00:00', 3, 4.5, 's'),
 	(NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL),
 	(NULL, '+Inf', NULL, NULL, NULL, NULL, NULL, NULL, NULL, 'Infinity', NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL),
 	(NULL, '-Inf', NULL, NULL, NULL, NULL, NULL, NULL, NULL, '-Infinity', NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL),
 	(NULL, 'NaN', NULL, NULL, NULL, NULL, NULL, NULL, NULL, 'NaN', NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL);
 ----
 ----
-
diff --git a/pkg/cmd/cmp-protocol/main.go b/pkg/cmd/cmp-protocol/main.go
@@ -64,6 +64,9 @@ func main() {
 					sqlbase.ColumnType_COLLATEDSTRING, // pg complains about utf8
 					sqlbase.ColumnType_INT2VECTOR,
 					sqlbase.ColumnType_OIDVECTOR,
+					sqlbase.ColumnType_OID,         // our 8-byte ints are usually out of range for pg
+					sqlbase.ColumnType_FLOAT,       // slight rounding differences at the end
+					sqlbase.ColumnType_TIMESTAMPTZ, // slight timezone differences
 					// tested manually below:
 					sqlbase.ColumnType_ARRAY,
 					sqlbase.ColumnType_TUPLE:
@@ -74,11 +77,11 @@ func main() {
 					continue
 				}
 				for _, format := range []string{
-					"SELECT %s;",
-					"SELECT ARRAY[%s];",
-					"SELECT (%s, NULL);",
+					"SELECT %s::%s;",
+					"SELECT ARRAY[%s::%s];",
+					"SELECT (%s::%s, NULL);",
 				} {
-					input := fmt.Sprintf(format, datum)
+					input := fmt.Sprintf(format, datum, pgTypeName(sem))
 					stmtCh <- input
 					fmt.Printf("\nTYP: %v, DATUM: %v\n", sem, datum)
 				}
@@ -89,12 +92,27 @@ func main() {
 	for input := range stmtCh {
 		fmt.Println("INPUT", input)
 		if err := compare(os.Stdout, input, *pgAddr, *crAddr, *pgUser, *crUser); err != nil {
-			fmt.Printf("sql: %s\n%+v\n", input, err)
-			os.Exit(1)
+			fmt.Fprintln(os.Stderr, "ERROR:", input)
+			fmt.Fprintf(os.Stderr, "%v\n", err)
+		} else {
+			fmt.Fprintln(os.Stderr, "OK", input)
 		}
 	}
 }
 
+func pgTypeName(sem sqlbase.ColumnType_SemanticType) string {
+	switch sem {
+	case sqlbase.ColumnType_STRING:
+		return "TEXT"
+	case sqlbase.ColumnType_BYTES:
+		return "BYTEA"
+	case sqlbase.ColumnType_INT:
+		return "INT8"
+	default:
+		return sem.String()
+	}
+}
+
 func compare(w io.Writer, input, pgAddr, crAddr, pgUser, crUser string) error {
 	ctx := context.Background()
 	for _, code := range []pgwirebase.FormatCode{

diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go
@@ -33,6 +33,7 @@ import (
 	"flag"
 	"fmt"
 	"log"
+	"math"
 	"os"
 	"sort"
 	"text/template"
@@ -82,12 +83,17 @@ func main() {
 
 	for _, expr := range stmts {
 		sql := fmt.Sprintf("SELECT %s", expr)
+		text, err := pgconnect.Connect(ctx, sql, *postgresAddr, *postgresUser, pgwirebase.FormatText)
+		if err != nil {
+			log.Fatalf("text: %s: %v", sql, err)
+		}
 		binary, err := pgconnect.Connect(ctx, sql, *postgresAddr, *postgresUser, pgwirebase.FormatBinary)
 		if err != nil {
-			log.Println(err)
+			log.Fatalf("binary: %s: %v", sql, err)
 		}
 		data = append(data, entry{
 			SQL:    expr,
+			Text:   text,
 			Binary: binary,
 		})
 	}
@@ -103,6 +109,7 @@ func main() {
 
 type entry struct {
 	SQL    string
+	Text   []byte
 	Binary []byte
 }
 
@@ -124,6 +131,8 @@ const outputJSON = `[
 	{{- if gt $idx 0 }},{{end}}
 	{
 		"SQL": {{.SQL | json}},
+		"Text": {{printf "%q" .Text}},
+		"TextAsBinary": {{.Text | binary}},
 		"Binary": {{.Binary | binary}}
 	}
 {{- end}}
@@ -132,6 +141,7 @@ const outputJSON = `[
 
 var inputs = map[string][]string{
 	"'%s'::decimal": {
+		"NaN",
 		"-000.000",
 		"-0000021234.23246346000000",
 		"-1.2",
@@ -185,21 +195,68 @@ var inputs = map[string][]string{
 		"42",
 	},
 
+	"'%s'::float8": {
+		// The Go binary encoding of NaN differs from Postgres by a 1 at the
+		// end. Go also uses Inf instead of Infinity (used by Postgres) for text
+		// float encodings. These deviations are still correct, and it's not worth
+		// special casing them into the code, so they are commented out here.
+		//"NaN",
+		//"Inf",
+		//"-Inf",
+		"-000.000",
+		"-0000021234.23246346000000",
+		"-1.2",
+		".0",
+		".1",
+		".1234",
+		".12345",
+		fmt.Sprint(math.MaxFloat32),
+		fmt.Sprint(math.SmallestNonzeroFloat32),
+		fmt.Sprint(math.MaxFloat64),
+		fmt.Sprint(math.SmallestNonzeroFloat64),
+	},
+
 	"'%s'::timestamp": {
+		"1999-01-08 04:05:06+00",
+		"1999-01-08 04:05:06+00:00",
+		"1999-01-08 04:05:06+10",
+		"1999-01-08 04:05:06+10:00",
+		"1999-01-08 04:05:06+10:30",
+		"1999-01-08 04:05:06",
+		"2004-10-19 10:23:54",
+		"0001-01-01 00:00:00",
+		"0004-10-19 10:23:54",
+		"0004-10-19 10:23:54 BC",
+		"4004-10-19 10:23:54",
+		"9004-10-19 10:23:54",
+	},
+
+	/* TODO(mjibson): fix these; there's a slight timezone display difference
+	"'%s'::timestamptz": {
+		"1999-01-08 04:05:06+00",
+		"1999-01-08 04:05:06+00:00",
+		"1999-01-08 04:05:06+10",
+		"1999-01-08 04:05:06+10:00",
+		"1999-01-08 04:05:06+10:30",
 		"1999-01-08 04:05:06",
 		"2004-10-19 10:23:54",
 		"0001-01-01 00:00:00",
 		"0004-10-19 10:23:54",
+		"0004-10-19 10:23:54 BC",
 		"4004-10-19 10:23:54",
 		"9004-10-19 10:23:54",
 	},
+	*/
 
 	"'%s'::date": {
 		"1999-01-08",
 		"0009-01-08",
 		"9999-01-08",
 		"1999-12-30",
 		"1996-02-29",
+		"0001-01-01",
+		"0001-01-01 BC",
+		"3592-12-31 BC",
 	},
 
 	"'%s'::time": {
@@ -232,6 +289,23 @@ var inputs = map[string][]string{
 		"-1y",
 		"-1y1mon",
 		"-1y1mon10s",
+		"1ms",
+		".2ms",
+		".003ms",
+		"-6s2ms",
+		"-1d6s2ms",
+		"-1d -6s2ms",
+		"-1mon1m",
+		"-1mon -1m",
+		"-1d1m",
+		"-1d -1m",
+		"-1y1m",
+		"-1y -1m",
+		"3y4mon5d6ms",
+		"296537y20d15h30m7s",
+		"-2965y -20d -15h -30m -7s",
+		"00:00:00",
+		"-00:00:00",
 	},
 
 	"'%s'::inet": {
@@ -312,4 +386,32 @@ var inputs = map[string][]string{
 		"000000001",
 		"0010101000011010101111100100011001110101100001010101",
 	},
+
+	"array[%s]::text[]": {
+		`NULL`,
+		`''`,
+		`'test'`,
+		`'test with spaces'`,
+		`e'\f'`,
+		// byte order mark
+		`e'\uFEFF'`,
+		// snowman
+		`e'\u2603'`,
+	},
+
+	"array[%s]": {
+		`''`,
+		`'\x0d53e338548082'::BYTEA`,
+		`'test with spaces'::BYTEA`,
+		`'name'::NAME`,
+	},
+
+	"(%s,null)": {
+		`1::int8,2::int8,3::int8,4::int8`,
+		`'test with spaces'::BYTEA`,
+		`'test with spaces'::TEXT`,
+		`'name'::NAME`,
+		`'false'::JSONB`,
+		`'{"a": []}'::JSONB`,
+	},
 }
diff --git a/pkg/keys/printer_test.go b/pkg/keys/printer_test.go
@@ -163,10 +163,10 @@ func TestPrettyPrint(t *testing.T) {
 			"/Table/42/B0000000000000000000000000000000000000000000000000000000000000000/PrefixEnd"},
 		{makeKey(MakeTablePrefix(42),
 			roachpb.RKey(durationAsc)),
-			"/Table/42/1mon1d1s"},
+			"/Table/42/1 mon 1 day 00:00:01"},
 		{makeKey(MakeTablePrefix(42),
 			roachpb.RKey(durationDesc)),
-			"/Table/42/-2mon-2d743h59m58s999ms999µs999ns"},
+			"/Table/42/-2 mons -2 days +743:59:58.999999999"},
 
 		// sequence
 		{MakeSequenceKey(55), `/Table/55/1/0/0`},

diff --git a/pkg/roachpb/data_test.go b/pkg/roachpb/data_test.go
@@ -1349,7 +1349,7 @@ func TestValuePrettyPrint(t *testing.T) {
 		{floatValue, "/FLOAT/6.28"},
 		{timeValue, "/TIME/2016-06-29T16:02:50.000000005Z"},
 		{decimalValue, "/DECIMAL/6.28"},
-		{durationValue, "/DURATION/1mon2d3ns"},
+		{durationValue, "/DURATION/1 mon 2 days 00:00:00.000000003"},
 		{MakeValueFromBytes([]byte{0x1, 0x2, 0xF, 0xFF}), "/BYTES/0x01020fff"},
 		{MakeValueFromString("foo"), "/BYTES/foo"},
 		{tupleValue, "/TUPLE/1:1:Int/8/2:3:Bytes/foo"},

diff --git a/pkg/server/settingsworker_test.go b/pkg/server/settingsworker_test.go
@@ -219,7 +219,7 @@ func TestSettingsSetAndShow(t *testing.T) {
 	if expected, actual := time.Hour*2, durationA.Get(&st.SV); expected != actual {
 		t.Fatalf("expected %v, got %v", expected, actual)
 	}
-	if expected, actual := "2h", db.QueryStr(t, fmt.Sprintf(showQ, durationKey))[0][0]; expected != actual {
+	if expected, actual := "02:00:00", db.QueryStr(t, fmt.Sprintf(showQ, durationKey))[0][0]; expected != actual {
 		t.Fatalf("expected %v, got %v", expected, actual)
 	}
 

diff --git a/pkg/sql/descriptor_mutation_test.go b/pkg/sql/descriptor_mutation_test.go
@@ -987,8 +987,8 @@ CREATE TABLE t.test (a STRING PRIMARY KEY, b STRING, c STRING, INDEX foo (c));
 	mt.CheckQueryResults(t,
 		"SHOW COLUMNS FROM t.test",
 		[][]string{
-			{"a", "STRING", "false", "NULL", "", "{\"primary\",\"ufo\"}", "false"},
-			{"d", "STRING", "true", "NULL", "", "{\"ufo\"}", "false"},
+			{"a", "STRING", "false", "NULL", "", "{primary,ufo}", "false"},
+			{"d", "STRING", "true", "NULL", "", "{ufo}", "false"},
 			{"e", "STRING", "true", "NULL", "", "{}", "false"},
 		},
 	)

diff --git a/pkg/sql/logictest/testdata/logic_test/aggregate b/pkg/sql/logictest/testdata/logic_test/aggregate
@@ -561,7 +561,7 @@ NULL NULL NULL NULL
 query TT
 SELECT array_agg(k), array_agg(s) FROM (SELECT k, s FROM kv ORDER BY k)
 ----
-{1,3,5,6,7,8} {"a","a",NULL,"b","b","A"}
+{1,3,5,6,7,8}  {a,a,NULL,b,b,A}
 
 query T
 SELECT array_agg(k) || 1 FROM (SELECT k FROM kv ORDER BY k)
@@ -660,7 +660,7 @@ INSERT INTO intervals VALUES (INTERVAL '1 year 2 months 3 days 4 seconds'), (INT
 query T
 SELECT sum(a) FROM intervals
 ----
-3y5mon7d19s
+3 years 5 mons 7 days 00:00:19
 
 
 query error unknown signature: avg\(string\)
@@ -1160,9 +1160,9 @@ SELECT 1 FROM kv GROUP BY v, w::DECIMAL HAVING w::DECIMAL > 1
 query IT rowsort
 SELECT v, array_agg('a') FROM kv GROUP BY v
 ----
-2     {"a","a","a"}
-4     {"a","a"}
-NULL  {"a"}
+2     {a,a,a}
+4     {a,a}
+NULL  {a}
 
 # Regression test for #26419
 query I

diff --git a/pkg/sql/logictest/testdata/logic_test/alter_column_type b/pkg/sql/logictest/testdata/logic_test/alter_column_type
@@ -24,12 +24,12 @@ ALTER TABLE t ALTER s TYPE BYTES, ALTER sl TYPE STRING(6), ALTER ts TYPE TIMESTA
 query TTBTTTB colnames
 SHOW COLUMNS FROM t
 ----
-column_name  data_type    is_nullable  column_default  generation_expression  indices      is_hidden
-s            BYTES        true         NULL            ·                      {}           false
-sl           STRING(6)    true         NULL            ·                      {}           false
-t            TIME         true         NULL            ·                      {}           false
-ts           TIMESTAMPTZ  true         NULL            ·                      {}           false
-rowid        INT          false        unique_rowid()  ·                      {"primary"}  true
+column_name  data_type    is_nullable  column_default  generation_expression  indices    is_hidden
+s            BYTES        true         NULL            ·                      {}         false
+sl           STRING(6)    true         NULL            ·                      {}         false
+t            TIME         true         NULL            ·                      {}         false
+ts           TIMESTAMPTZ  true         NULL            ·                      {}         false
+rowid        INT          false        unique_rowid()  ·                      {primary}  true
 
 query TTTT
 SELECT * FROM t
@@ -108,19 +108,19 @@ CREATE TABLE t (a INT)
 query TTBTTTB colnames
 SHOW COLUMNS FROM t
 ----
-column_name  data_type  is_nullable  column_default  generation_expression  indices      is_hidden
-a            INT        true         NULL            ·                      {}           false
-rowid        INT        false        unique_rowid()  ·                      {"primary"}  true
+column_name  data_type  is_nullable  column_default  generation_expression  indices    is_hidden
+a            INT        true         NULL            ·                      {}         false
+rowid        INT        false        unique_rowid()  ·                      {primary}  true
 
 statement ok
 ALTER TABLE t ALTER a TYPE INTEGER
 
 query TTBTTTB colnames
 SHOW COLUMNS FROM t
 ----
-column_name  data_type  is_nullable  column_default  generation_expression  indices      is_hidden
-a            INT        true         NULL            ·                      {}           false
-rowid        INT        false        unique_rowid()  ·                      {"primary"}  true
+column_name  data_type  is_nullable  column_default  generation_expression  indices    is_hidden
+a            INT        true         NULL            ·                      {}         false
+rowid        INT        false        unique_rowid()  ·                      {primary}  true
 
 statement ok
 DROP TABLE t