Skip to content

Commit

Permalink
sql/sem/tree: make function names properly case-sensitive
Browse files Browse the repository at this point in the history
There was a time in the past where CockroachDB was (or attempted to
be) case-insensitive about identifiers, and this was a source of
incompatibility with PostgreSQL.

This incompatibility was corrected for object
(table/view/database/index) names and column names in #16884, but
somehow I forgot about function names in that patch. As a result,
CockroachDB continued to consider `upper()` and `"UPPER"()` the same
thing, whereas most definitely only the former is valid.

This patch corrects the issue and simplifies the code accordingly.

Release note (sql change): CockroachDB now properly rejects
incorrectly cased SQL function names with an error.
  • Loading branch information
knz committed Feb 4, 2018
1 parent 07c88d5 commit 6bb56ab
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 21 deletions.
5 changes: 5 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/function_lookup
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@ query T
INSERT INTO foo(x) VALUES (42) RETURNING PG_TYPEOF(x)
----
int

# CockroachDB is case-preserving for quoted identifiers like pg, and
# function names only exist in lowercase.
query error unknown function: PG_TYPEOF()
SELECT "PG_TYPEOF"(123)
7 changes: 1 addition & 6 deletions pkg/sql/sem/builtins/all_builtins.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package builtins

import (
"sort"
"strings"

"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sem/types"
Expand All @@ -40,18 +39,14 @@ func init() {
AllBuiltinNames = append(AllBuiltinNames, name)
}

// We alias the builtins to uppercase to hasten the lookup in the
// common case. Also generate missing categories.
// Generate missing categories.
for _, name := range AllBuiltinNames {
uname := strings.ToUpper(name)
def := Builtins[name]
for i, b := range def {
if b.Category == "" {
def[i].Category = getCategory(def[i])
}
}
Builtins[uname] = def
tree.FunDefs[uname] = tree.FunDefs[name]
}

sort.Strings(AllBuiltinNames)
Expand Down
19 changes: 4 additions & 15 deletions pkg/sql/sem/tree/function_definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
package tree

import (
"strings"

"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
)
Expand Down Expand Up @@ -80,25 +78,16 @@ func (n *UnresolvedName) ResolveFunction(
return d, nil
}

// Although the conversion from Name to string should go via
// Name.Normalize(), functions are special in that they are
// guaranteed to not contain special Unicode characters. So we can
// use ToLower directly.
// TODO(knz): this will need to be revisited once we allow
// function names to exist in custom namespaces, whose names
// may contain special characters.
prefix = strings.ToLower(prefix)
smallName := strings.ToLower(function)
fullName := smallName
fullName := function

if prefix == "pg_catalog" {
if prefix == sessiondata.PgCatalogName {
// If the user specified e.g. `pg_catalog.max()` we want to find
// it in the global namespace.
prefix = ""
}

if prefix != "" {
fullName = prefix + "." + smallName
fullName = prefix + "." + function
}
def, ok := FunDefs[fullName]
if !ok {
Expand All @@ -108,7 +97,7 @@ func (n *UnresolvedName) ResolveFunction(
// the search path first.
iter := searchPath.Iter()
for alt, ok := iter(); ok; alt, ok = iter() {
fullName = alt + "." + smallName
fullName = alt + "." + function
if def, ok = FunDefs[fullName]; ok {
found = true
break
Expand Down

0 comments on commit 6bb56ab

Please sign in to comment.