Cann't get data while table has column named 'name'

[veteranlu]

My test case: CREATE TABLE t1 (name INT PRIMARY KEY, c2 CHAR(10)); INSERT INTO t1 VALUES(1, 'AAA'); SELECT * FROM t1;

[vivekmenezes]

Thanks for the report kaka. Do you think you can fix this bug? Thanks

On Wed, Aug 5, 2015, 9:09 PM kaka lu notifications@github.com wrote:

My test case: CREATE TABLE t1 (name INT PRIMARY KEY, c2 CHAR(10)); INSERT INTO t1 VALUES(1, 'AAA'); SELECT * FROM t1;

— Reply to this email directly or view it on GitHub https://github.com/cockroachdb/cockroach/issues/1986.

[veteranlu]

@vivekmenezes , let me try :)

[arypurnomoz]

It's not just "name". "domain" and some common reserved word in sql seems also cannot be used as a column name

[petermattis]

NAME and DOMAIN are both unreserved_keywords in the postgres grammar. We've removed the parts of the grammar that use these keywords (ALTER DOMAIN and the xml stuff) and can likely just remove them as tokens completely now.

[mberhault]

this is encodeSQLIdent adding escaped quotes around all keywords. we could probably have it apply to only certain levels of keyword reserved-ness

[mberhault]

for info, here is a full sql logic test showing the failure:

statement ok
CREATE TABLE t1 (name INT PRIMARY KEY, c2 CHAR(10))

statement ok
INSERT INTO t1 VALUES(1, 'AAA');

query TTT colnames
SHOW COLUMNS FROM t1
----
Field Type     Null
name  INT      true
c2    CHAR(10) true

statement error column ""name"" not found
SELECT * FROM t1;

[veteranlu]

@MBerhault u r right.

The error reported by parser.EvalExpr, because the Expr of scanNode.render added escaped quotes, but the column name of scanNode.vals don't add escaped quotes.

We can fix this bug with follow solution:

1. Add escaped quotes with column name which is reserved key while add to scanNode.vals, just like this.

name := encIdent(col.Name)
n.vals[name] = unmarshalValue(*col, kv)

1. Add escaped quotes with column which is reserved key while insert table

Which one is better?

[petermattis]

@veteranlu A column name, even if it is a reserved word, is just the name without quotes, so I don't think the first option is correct. I think the problem is that EvalExpr has overly simplistic logic for looking up QualifiedName. Right now it just calls QualifiedName.String() and passes the result to Env.Get(). But it should really probably pass the QualifiedName itself to Env.Get(). Doing that is a somewhat invasive change.

Even better, though, would be to get rid of Env entirely. Now that we can walk an expression and find all of the QualifiedName nodes, we should perform a pass before expression evaluation in which we replace QualifiedName with some sort of new DRef node. This would allow statement execution to properly map a QualifiedName to the correct column in cases where the name actually is qualified (e.g. SELECT foo.bar FROM foo) and allow faster expression evaluation since we could avoid the map lookup. This change is even more invasive than changing the signature of Env.Get(), though. Happy to provide more details if you still want to tackle fixing this.

[veteranlu]

@petermattis I will check postgresql's implement， maybe there are some clues.

[veteranlu]

@petermattis postgresql wouldn't quote unreserved keyword.

/* Column identifier --- names that can be column, table, etc names.
 */
ColId:      IDENT                                   { $$ = $1; }
            | unreserved_keyword                    { $$ = pstrdup($1); }
            | col_name_keyword                      { $$ = pstrdup($1); }

cockroach add quote for all kind of keyword.

    if _, ok := keywords[strings.ToUpper(s)]; ok {
        fmt.Fprintf(buf, "\"%s\"", s)
        return
    }

Maybe we mixed reserved and unreserved keyword, we should add type for keyword, like postgresql:

PG_KEYWORD("ALTER", ALTER, UNRESERVED_KEYWORD)
PG_KEYWORD("ALWAYS", ALWAYS, UNRESERVED_KEYWORD)
PG_KEYWORD("ANALYSE", ANALYSE, RESERVED_KEYWORD)        /* British spelling */
PG_KEYWORD("ANALYZE", ANALYZE, RESERVED_KEYWORD)

so we just only quote reserved keyword as @MBerhault said.

[petermattis]

I'm fine with adding a type field to the keywords map. Or to add a separate reservedKeywords map.

[tamird]

Looks like this is fixed now.

> create database t;
OK
> set database = t;
OK
> CREATE TABLE t1 (name INT PRIMARY KEY, c2 CHAR(10));
OK
> INSERT INTO t1 VALUES(1, 'AAA');
OK
> SELECT * FROM t1;
name    c2
1   AAA

[petermattis]

The qualified name lookup was fixed by #2021. Someone still needs to fix the unnecessary quoting of non_reserved_keywords.

[tamird]

Meaning separating sql/parser/keywords into keywords and sql/parser/reservedKeywords? What's the symptom that would address?

[petermattis]

Its a minor enhancement now: SELECT name FROM bar currently is output as SELECT "name" FROM bar. I'll file a separate issue.