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
Allow writing table name as a string literal #52635
Allow writing table name as a string literal #52635
Conversation
Could you please update AST Fuzzer so it will replace table name identifiers with string literals sometimes? |
@@ -188,6 +188,20 @@ bool ParserIdentifier::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) | |||
++pos; | |||
return true; | |||
} | |||
else if (pos->type == TokenType::StringLiteral) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a complication: this code allows parsing a string literal as an identifier in every context. It can be undesirable. I expect to allow it only when we expect a table name.
ReadBufferFromMemory buf(pos->begin, pos->size()); | ||
String s; | ||
|
||
readQuotedStringWithSQLStyle(s, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a low-level function, but we also have ParserStringLiteral; maybe it will fit better.
This is an automated comment for commit b53084c with description of existing statuses. It's updated for the latest CI running
|
@hendrik-m Let's check tests failures. |
I've checked and think this does not seem to be the right approach. In the AST the quotes are parsed and therefore gone. In order to fuzz quoting styles, the dump of the AST which happens here has to be extended to change the quoting style in FormatSettings at random. However the quoting style added for this PR only targets table names, so it gets a bit more complex:
|
to `clickhouse-local`
Looks perfect :) |
|
||
```bash | ||
./clickhouse local -q "SELECT * FROM file('reviews.tsv')" | ||
./clickhouse-local -q "SELECT * FROM 'reviews.tsv'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JFYI, the reason why instead of clickhouse-local it was written as ./clickhouse local
is as follows:
Our documentation engineers (all two of them) are using Mac.
In Mac, the ClickHouse installation script Install.cpp
does not support creating symlinks and installing into /usr/bin/
But ClickHouse download script will put the clickhouse
binary into the current directory, and it can be run without installation.
Although, I figured out, how to install ClickHouse on Mac here: https://github.com/ClickHouse/NoiSQL#quick-start---macos
we did not put it into the Install script because no one among the engineers is using Mac, so the development iterations for Mac installation can be long.
Your edit does not look right because it is unlikely that the symlink exists, but clickhouse is not installed in PATH.
|
||
try | ||
{ | ||
readQuotedStringWithSQLStyle(s, in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tryReadQuotedStringInto
- we can avoid throwing and catching exceptions here.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add support for string literals as table name. Closes #52178
Documentation entry for user-facing changes
Documentation is written (mandatory for new features)
Motivation: Why is this function, table engine, etc. useful to ClickHouse users?
Supporting string literals is more convenient to use as it does not require quoting.
Example use: A query or command.
clickhouse-local --query "SELECT * FROM 'dataset.avro'"