Skip to content

Commit

Permalink
sql/types, sql/stats: fix SQLStringFullyQualified for arrays of UDTs
Browse files Browse the repository at this point in the history
Starting in v24.2 we added `SQLStringFullyQualified`, but it looks like
it didn't work for arrays of user-defined types. This is only used in a
few places, including the output of `SHOW STATISTICS USING JSON`.

This commit:
1. fixes `SQLStringFullyQualified` for arrays of UDTs
2. adds the `t.TypeMeta.Name == nil` guard to `SQLString` for composite
   UDTs
3. sets a formatting flag in `stats.(*JSONStatistic).SetHistogram` so
   that we get fully-qualified type annotations on histogram upper
   bounds

No release note because the output of `SHOW STATISTICS USING JSON` isn't
documented.

Fixes: #137443

Release note: None
  • Loading branch information
michae2 committed Dec 23, 2024
1 parent 3b46aff commit b6be9de
Show file tree
Hide file tree
Showing 5 changed files with 162 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ FROM (
)
----
["e"] "mydb.public.e" "x"
["a"] "public.e[]" "ARRAY['y':::public.e,'z':::public.e]"
["a"] "mydb.public.e[]" "ARRAY['y':::mydb.public.e,'z':::mydb.public.e]"

query-sql cluster=c2
SELECT * FROM mydb.public.t
Expand Down
138 changes: 136 additions & 2 deletions pkg/sql/logictest/testdata/logic_test/distsql_stats
Original file line number Diff line number Diff line change
Expand Up @@ -1107,10 +1107,10 @@ statement ok
CREATE TYPE e AS ENUM ('hello', 'howdy', 'hi');

statement ok
CREATE TABLE et (x e, y e, PRIMARY KEY (x));
CREATE TABLE et (x e, y e, z e[], PRIMARY KEY (x), FAMILY (x, y, z));

statement ok
INSERT INTO et VALUES ('hello', 'hello'), ('howdy', 'howdy'), ('hi', 'hi');
INSERT INTO et VALUES ('hello', 'hello', '{hello}'), ('howdy', 'howdy', '{howdy}'), ('hi', 'hi', '{hi}');

statement ok
CREATE STATISTICS s FROM et
Expand All @@ -1130,6 +1130,140 @@ ORDER BY
statistics_name column_names row_count null_count has_histogram
s {x} 3 0 true
s {y} 3 0 true
s {z} 3 0 true

query T
SELECT jsonb_pretty(
regexp_replace(COALESCE(json_agg(stat), '[]')::STRING, '"id": [0-9]+', '"id": 0', 'g')::JSONB
)
FROM (
SELECT json_array_elements(statistics) - 'created_at' AS stat
FROM [SHOW STATISTICS USING JSON FOR TABLE et]
)
----
[
{
"avg_size": 4,
"columns": [
"x"
],
"distinct_count": 3,
"histo_buckets": [
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "hello"
},
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "howdy"
},
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "hi"
}
],
"histo_col_type": "test.public.e",
"histo_version": 3,
"id": 0,
"name": "s",
"null_count": 0,
"row_count": 3
},
{
"avg_size": 3,
"columns": [
"y"
],
"distinct_count": 3,
"histo_buckets": [
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "hello"
},
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "howdy"
},
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "hi"
}
],
"histo_col_type": "test.public.e",
"histo_version": 3,
"id": 0,
"name": "s",
"null_count": 0,
"row_count": 3
},
{
"avg_size": 7,
"columns": [
"z"
],
"distinct_count": 3,
"histo_buckets": [
{
"distinct_range": 0,
"num_eq": 1,
"num_range": 0,
"upper_bound": "ARRAY['hello':::test.public.e]"
},
{
"distinct_range": 1,
"num_eq": 1,
"num_range": 1,
"upper_bound": "ARRAY['hi':::test.public.e]"
}
],
"histo_col_type": "test.public.e[]",
"histo_version": 3,
"id": 0,
"name": "s",
"null_count": 0,
"row_count": 3
}
]

# Verify that we can inject these stats.
let $json_stats
SHOW STATISTICS USING JSON FOR TABLE et

statement ok
DELETE FROM system.table_statistics

# Restore the old stats.
statement ok
ALTER TABLE et INJECT STATISTICS $$$json_stats$$

query TTIIB colnames
SELECT
statistics_name,
column_names,
row_count,
null_count,
histogram_id IS NOT NULL AS has_histogram
FROM
[SHOW STATISTICS FOR TABLE et]
ORDER BY
column_names::STRING, created
----
statistics_name column_names row_count null_count has_histogram
s {x} 3 0 true
s {y} 3 0 true
s {z} 3 0 true

# JSON and other inverted-index columns. See also #35150.
statement ok
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/stats/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func (js *JSONStatistic) SetHistogram(h *HistogramData) error {
NumEq: b.NumEq,
NumRange: b.NumRange,
DistinctRange: b.DistinctRange,
UpperBound: tree.AsStringWithFlags(datum, tree.FmtExport),
UpperBound: tree.AsStringWithFlags(datum, tree.FmtExport|tree.FmtAlwaysQualifyUserDefinedTypeNames),
})
}
return nil
Expand Down
25 changes: 19 additions & 6 deletions pkg/sql/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -2037,6 +2037,13 @@ func (t *T) SQLString() string {
return t.TypeMeta.Name.FQName(false /* explicitCatalog */)
case TupleFamily:
if t.UserDefined() {
// We do not expect to be in a situation where we want to format a
// user-defined type to a string and do not have the TypeMeta hydrated,
// but there have been bugs in the past, and returning a less informative
// string is better than a nil-pointer panic.
if t.TypeMeta.Name == nil {
return fmt.Sprintf("@%d", t.Oid())
}
// Do not include the catalog name. We do not allow a table to reference
// a type in another database, so it will always be for the current database.
// Removing the catalog name makes the output more portable for other
Expand All @@ -2057,12 +2064,18 @@ func (t *T) SQLString() string {
// SQLStringFullyQualified is a wrapper for SQLString() for when we need the
// type name to be a fully-qualified 3-part name.
func (t *T) SQLStringFullyQualified() string {
if t.TypeMeta.Name != nil &&
(t.Family() == EnumFamily || (t.Family() == TupleFamily && t.UserDefined())) {
// Include the catalog in the type name. This is necessary to properly
// resolve the type, as some code paths require the database name to
// correctly distinguish cross-database references.
return t.TypeMeta.Name.FQName(true /* explicitCatalog */)
if t.UserDefined() {
switch t.Family() {
case ArrayFamily:
return t.ArrayContents().SQLStringFullyQualified() + "[]"
default:
if t.TypeMeta.Name != nil {
// Include the catalog in the type name. This is necessary to properly
// resolve the type, as some code paths require the database name to
// correctly distinguish cross-database references.
return t.TypeMeta.Name.FQName(true /* explicitCatalog */)
}
}
}
return t.SQLString()
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/sql/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1175,11 +1175,15 @@ func TestDelimiter(t *testing.T) {

// Prior to the patch which introduced this test, the below calls would
// have panicked.
func TestEnumWithoutTypeMetaNameDoesNotPanicInSQLString(t *testing.T) {
func TestUDTWithoutTypeMetaNameDoesNotPanicInSQLString(t *testing.T) {
typ := MakeEnum(100100, 100101)
require.Equal(t, "@100100", typ.SQLString())
arrayType := MakeArray(typ)
require.Equal(t, "@100100[]", arrayType.SQLString())
compositeType := NewCompositeType(100200, 100201, nil, nil)
require.Equal(t, "@100200", compositeType.SQLString())
arrayCompositeType := MakeArray(compositeType)
require.Equal(t, "@100200[]", arrayCompositeType.SQLString())
}

func TestSQLStringForError(t *testing.T) {
Expand Down

0 comments on commit b6be9de

Please sign in to comment.