Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql/parser: Add FAMILY as reserved keyword to avoid ambiguity #7069

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Jun 6, 2016

Fixes #7061.

This change adds FAMILY as a reserved keyword.

This is necessary to resolve ambiguity with our grammar. The ambiguity
is an artifact of FAMILY not being a reserved keyword. Without adding to
our reserved keywords or using multiple character lookahead, the
previous syntax was throwing a shift/reduce error. The ambiguity caused
by this error is shown with the following situation:

CREATE TABLE t (x type_t (opts));
RENAME t.x TO t.family;
SHOW CREATE TABLE t;

+-------+---------------------------------------+
| Table |                CreateTable            |
+-------+---------------------------------------+
| t     | CREATE TABLE t (family type_t (opts)) |
+-------+---------------------------------------+

Now that FAMILY is a reserved keyword, family will not be a valid
column name, and therefore the rename will not be possible.

@knz


This change is Reviewable

@nvanbenschoten nvanbenschoten added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-todo labels Jun 6, 2016
@tamird
Copy link
Contributor

tamird commented Jun 6, 2016

mechanics :lgtm: but I'm not sure about the choice of AS.

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

sql/parser: Change syntax of named col family to avoid ambiguity

Fixes #7061.

This change replaces the syntax of column family declarations from

FAMILY name (column) to FAMILY (column) AS name.

This is necessary to resolve ambiguity with our grammar. The ambiguity is

an artifact of FAMILY not being a reserved keyword. Without adding to

our reserved keywords or using multiple character lookahead, the

previous syntax was throwing a shift/reduce error. The ambiguity caused

by this error is shown with the following situation:


CREATE TABLE t (x type_t (opts));

RENAME t.x TO t.family;

SHOW CREATE TABLE;



+-------+---------------------------------------+

| Table |                CreateTable            |

+-------+---------------------------------------+

| t     | CREATE TABLE t (family type_t (opts)) |

+-------+---------------------------------------+

@knz


Reviewed 5 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@petermattis
Copy link
Collaborator

Any reason to not make FAMILY a reserved keyword?

@knz
Copy link
Contributor

knz commented Jun 6, 2016

@petermattis we discussed that. I argued that as a general matter of PL design we should aim to minimize the number of reserved keywords. Also, "family" seems a rather common name that users are likely to use for columns, doesn't it?

@petermattis
Copy link
Collaborator

petermattis commented Jun 6, 2016

@knz I agree that we should minimize the number of reserved keywords, but we also want to minimize the number of constructs and FAMILY (cols) AS bar is introducing a new use for AS to the CREATE TABLE statement.

PS A lot of people habitually quote SQL identifiers using double quotes (e.g. "family") to avoid conflicts with reserved words.

@knz
Copy link
Contributor

knz commented Jun 6, 2016

Well if you know from experience that people tend to double quote identifiers naturally, my main argument goes away then. Then the only point remaining is that we should highlight in our docs that we have more keywords.

@nvanbenschoten
Copy link
Member Author

nvanbenschoten commented Jun 6, 2016

A major benefit in my mind of making FAMILY a reserved keyword is that we can retain the syntactic parallels in the CREATE TABLE statement between INDEX, CONSTRAINT, and FAMILY (with optional names). What do you think @knz?

@knz
Copy link
Contributor

knz commented Jun 6, 2016

There are benefits to homogeneity but the higher level issue is to minimize inconvenience. Unless users are ready to accept more keywords (which they are generally not enclined to in other PLs but peter thinks for SQL it's OK) in general changing a regular identifier to a reserved keyword causes more inconvenience than additional grammar rules.

Fixes cockroachdb#7061.

This change adds `FAMILY` as a reserved keyword.

This is necessary to resolve ambiguity with our grammar. The ambiguity
is an artifact of `FAMILY` not being a reserved keyword. Without adding to
our reserved keywords or using multiple character lookahead, the
previous syntax was throwing a shift/reduce error. The ambiguity caused
by this error is shown with the following situation:

```
CREATE TABLE t (x type_t (opts));
RENAME t.x TO t.family;
SHOW CREATE TABLE t;

+-------+---------------------------------------+
| Table |                CreateTable            |
+-------+---------------------------------------+
| t     | CREATE TABLE t (family type_t (opts)) |
+-------+---------------------------------------+
```

Now that `FAMILY` is a reserved keyword, `family` will not be a valid
column name, and therefore the rename will not be possible.
@nvanbenschoten nvanbenschoten changed the title sql/parser: Change syntax of named col family to avoid ambiguity sql/parser: Add FAMILY as reserved keyword to avoid ambiguity Jun 6, 2016
@nvanbenschoten
Copy link
Member Author

Discussed with @knz in person and decided that the reserved keyword approach was ok. PTAL.

@petermattis
Copy link
Collaborator

:lgtm:

Another possibility: we decided on the term family in preference to group, yet group is already a reserved keyword.

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Discussed with @knz in person and decided that the reserved keyword approach was ok. PTAL.


Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@danhhz
Copy link
Contributor

danhhz commented Jun 6, 2016

:LGTM:

AFAIR, we switched to family largely because group was already used for something, which hasn't changed. So my preference is to stick with family.

Previously, petermattis (Peter Mattis) wrote…

:lgtm:

Another possibility: we decided on the term family in preference to group, yet group is already a reserved keyword.


Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


sql/parser/sql_union.sh, line 13 [r2] (raw file):

# compiler does not type check Go code.
function type_check() {
    ! go tool yacc -o /dev/null -p sql sql.y | grep -F 'conflicts'

👍


Comments from Reviewable

@nvanbenschoten
Copy link
Member Author

TFTRs. I'll stick with @paperstreet's preference. If we want to change this later, we can always use the GROUP keyword instead and remove FAMILY as a reserved keyword.

@nvanbenschoten nvanbenschoten merged commit 17283ef into cockroachdb:master Jun 6, 2016
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/fixParsing branch June 6, 2016 20:23
@jseldess
Copy link
Contributor

jseldess commented Jul 6, 2016

Our grammar diagram lists FAMILY as a reserved keyword. I don't think more's needed in docs right now, but let me know if you disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql grammar is broken with a yacc shift/reduce conflict
6 participants