Skip to content

Commit

Permalink
builtins: fix error case of crdb_internal.hide_sql_constants
Browse files Browse the repository at this point in the history
Fix one of the error cases of `hide_sql_constants` to append an empty
string to the array, rather than returning an empty string. Also, add
`hide_sql_constants` back to sqlsmith now that cockroachdb#96555 is fixed.

Epic: None

Fixes: cockroachdb#97830

Release note: None
  • Loading branch information
michae2 committed Mar 1, 2023
1 parent 998b030 commit 9818625
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
3 changes: 0 additions & 3 deletions pkg/internal/sqlsmith/schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -513,9 +513,6 @@ var functions = func() map[tree.FunctionClass]map[oid.Oid][]function {
"crdb_internal.revalidate_unique_constraint",
"crdb_internal.request_statement_bundle",
"crdb_internal.set_compaction_concurrency",
// TODO(96555): Temporarily disable crdb_internal.hide_sql_constants,
// which produces internal errors for some valid inputs.
"crdb_internal.hide_sql_constants",
// TODO(#97097): Temporarily disable crdb_internal.fingerprint
// which produces internal errors for some valid inputs.
"crdb_internal.fingerprint",
Expand Down
5 changes: 5 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/builtin_function
Original file line number Diff line number Diff line change
Expand Up @@ -3736,6 +3736,11 @@ SELECT crdb_internal.hide_sql_constants(ARRAY('select 1', NULL, 'select ''hello'
----
{"SELECT _",NULL,"SELECT '_'",""}

query T
SELECT crdb_internal.hide_sql_constants(ARRAY['banana'])
----
{""}

query T
SELECT crdb_internal.hide_sql_constants('SELECT ''yes'' IN (''no'', ''maybe'', ''yes'')')
----
Expand Down
12 changes: 6 additions & 6 deletions pkg/sql/sem/builtins/builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -7718,7 +7718,7 @@ expires until the statement bundle is collected`,
return tree.NewDString(sqlNoConstants), nil
},
types.String,
"Removes constants from a SQL statement. String provided must contain at most"+
"Removes constants from a SQL statement. String provided must contain at most "+
"1 statement.",
volatility.Immutable,
),
Expand All @@ -7742,11 +7742,11 @@ expires until the statement bundle is collected`,

if len(sql) != 0 {
parsed, err := parser.ParseOne(sql)
if err != nil {
return tree.NewDString(sqlNoConstants), nil //nolint:returnerrcheck
// If parsing is unsuccessful, we append an empty string instead of
// returning an error.
if err == nil {
sqlNoConstants = tree.AsStringWithFlags(parsed.AST, tree.FmtHideConstants)
}

sqlNoConstants = tree.AsStringWithFlags(parsed.AST, tree.FmtHideConstants)
}

if err := result.Append(tree.NewDString(sqlNoConstants)); err != nil {
Expand All @@ -7756,7 +7756,7 @@ expires until the statement bundle is collected`,

return result, nil
},
Info: "Hide constants for each element in an array of SQL statements." +
Info: "Hide constants for each element in an array of SQL statements. " +
"Note that maximum 1 statement is permitted per string element.",
Volatility: volatility.Immutable,
},
Expand Down

0 comments on commit 9818625

Please sign in to comment.