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

Function names should be quoted sometimes. #3333

Closed
wants to merge 1 commit into from

Conversation

bramp
Copy link
Contributor

@bramp bramp commented Oct 21, 2017

In MySQL it is valid to have function names with characters which would not normally be allowed, as long as the function name is quoted. This change ensures the function name is quoted when needed, allowing reserved words to stay unquoted.

In MySQL it is valid to have function names with characters which would not normally be allowed, as long as the function is quoted. This change ensures the function name is quoted when needed, allowing reserved words to stay unquoted.
@bramp
Copy link
Contributor Author

bramp commented Oct 21, 2017

Actually hold off on this.

I've been fuzzing the sqlparser, and I hit examples like: SELECT`SET`(), which gets parsed correctly, but printed out like SELECT SET(), which we can't be re-parse, as SET is a reserved word. I think if I want to do this right, I need to use a list of reserved words to selectively escape.

How much does the vitess sqlparser want to adhere to the MySQL syntax? I see many reserved words are missing, and certain definitions are slightly wrong. For example, per the spec, identifiers are allowed to start with digits, but this parser disallows that.

@sougou
Copy link
Contributor

sougou commented Oct 24, 2017

I seem to have an explicit comment here: https://github.com/youtube/vitess/blob/master/go/vt/sqlparser/ast.go#L2203, where it says that function names should not be back-quoted.

I think it's because most functions are keywords, and they don't get backquoted, like INTERVAL(...). Are there cases where it is necessary to backquote?

@bramp
Copy link
Contributor Author

bramp commented Oct 24, 2017

It is only necessary if it is a user defined function, that is named the same as a reserved word, or has unusual characters.

For example the following is valid (yet ugly):

CREATE FUNCTION `hello``` (s CHAR(20)) RETURNS CHAR(50) DETERMINISTIC RETURN CONCAT('Hello, ',s,'!');

SELECT `hello```('world');

Anyway while fuzzing the parser, I got lots of false positives, with examples like SELECT SET() which could be parsed correctly, but the String(..) output (SELECT SET()) could not be re-parsed.

We could more strictly follow the the MySQL definition by only backtick quoting the function name, when it contains unusual characters, or when it is a reserved word.

Or we could just ignore this edge case.

@sougou
Copy link
Contributor

sougou commented Oct 24, 2017

My main concern against using back-ticks is the readability of the generated SQL. This has far-reaching impact. Even simple aggregates like select count(*) would become select `count`(*).

How about escaping a function name only if it has special characters?

If this is acceptable, we'll need to create a new type called FuncIdent that uses a variant of formatID.

@bramp
Copy link
Contributor Author

bramp commented Oct 24, 2017

sorry, yes to be clear, I would only escape if needed. so count(*) would remain unescaped, but set() would be escaped.

I'll change this commit, and give you some test cases so we are on the same page.

@sougou
Copy link
Contributor

sougou commented Jan 21, 2018

Ping: are you planning to work on this?

@bramp
Copy link
Contributor Author

bramp commented Jan 21, 2018

Sorry, I went down a rabbit hole trying to copy exactly how mysql does it, as there are a few edge cases, then I got distracted and moved on to something else. I would like to finish, but realistically I won't get to it soon. So feel free to close and I'll reopen if I return to it.

@sougou
Copy link
Contributor

sougou commented Jan 21, 2018

Sounds good. I'll close this for now and wait till you can get back to it.

@sougou sougou closed this Jan 21, 2018
frouioui pushed a commit to planetscale/vitess that referenced this pull request Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants