-
Notifications
You must be signed in to change notification settings - Fork 1.8k
datafusion-cli support for multiple commands in a single line #9831
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
datafusion-cli support for multiple commands in a single line #9831
Conversation
mustafasrepo
left a comment
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.
LGTM!. I wonder though how we can test this support in the code base.
alamb
left a comment
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 nice change -- thank you @berkaysynnada
| } | ||
|
|
||
| /// Splits a string which consists of multiple queries. | ||
| pub(crate) fn split_from_semicolon(sql: String) -> Vec<String> { |
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.
You can probably avoid a copy here if you passed in a reference
| pub(crate) fn split_from_semicolon(sql: String) -> Vec<String> { | |
| pub(crate) fn split_from_semicolon(sql: &str) -> Vec<&str> { |
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.
I cannot find a copy here. If pass the reference, I also need to copy it for exec_and_print().
datafusion-cli/src/helper.rs
Outdated
|
|
||
| /// Splits a string which consists of multiple queries. | ||
| pub(crate) fn split_from_semicolon(sql: String) -> Vec<String> { | ||
| sql.split(';') |
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 may want to add a test? Just wondering if it will work for query
select ';'
?
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.
I will send a commit excluding semicolons if they are in "" or '', and some tests.
alamb
left a comment
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.
Looks like an improvement to me. Thanks @berkaysynnada
| Ok(()) | ||
| } | ||
|
|
||
| #[test] |
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.
👍 thank you for the tests 👍
| let mut in_single_quote = false; | ||
| let mut in_double_quote = false; | ||
|
|
||
| for c in sql.chars() { |
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 likely pretty brittle (likely won't handle all crazy corner cases, like for example when there are escaped quotes \\', but I think it is better than what we have now.
Thanks @berkaysynnada
…#9831) * Multiple Create External Table's are supported from CLI * Handle in-quote semicolons * add test
Which issue does this PR close?
Closes #.
Rationale for this change
While we can write CLI commands sequentially such as:
CLI gives error to write more commands after "CREATE EXTERNAL TABLE":
🤔 Invalid statement: sql parser error: Expected end of statement, found: SELECTWhat changes are included in this PR?
This PR addresses the issue that we needed to enter commands one by one for creating external tables. It resolves this by splitting the entire string at semicolons, ensuring each segment passes parser checks one by one, and then executing them sequentially.
Are these changes tested?
Are there any user-facing changes?