-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Example for simple Expr --> SQL conversion #10528
Conversation
Using the
|
For anyone else following along, this conversation is happening on the ticket: #10524 (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.
Thanks @edmondop -- this is pretty sweet
It would also be good to add plan_to_sql.rs
to the list of examples in https://github.com/apache/datafusion/tree/main/datafusion-examples#single-process
Ok(()) | ||
} | ||
|
||
/// DataFusion can convert expressions to SQL |
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.
Love it!
Examples are now more complete and readme is updated |
Update file header with more details about the examples
datafusion-examples/README.md
Outdated
@@ -63,6 +63,7 @@ cargo run --example csv_sql | |||
- [`parquet_sql.rs`](examples/parquet_sql.rs): Build and run a query plan from a SQL statement against a local Parquet file | |||
- [`parquet_sql_multiple_files.rs`](examples/parquet_sql_multiple_files.rs): Build and run a query plan from a SQL statement against multiple local Parquet files | |||
- ['parquet_exec_visitor.rs'](examples/parquet_exec_visitor.rs): Extract statistics by visiting an ExecutionPlan after execution | |||
- - [`plan_to_sql.rs`](examples/plan_to_sql.rs): Generate SQL from Datafusion `Expr` and `LogicalPlan` |
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 seems an additional -
?
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 @edmondop -- this looks like a great start of these examples. I think @yyy1000's comments about the extra -
should be addressed but it isn't required.
cc @devinjdangelo and @backkem (given you started this project)
I filed #10550 for hte logical plan version too |
Fixing extra -
The current unparser is somewhat conservative for correctness sake; Always quoting identifiers and adding every set of parentheses possible. If we want to make the generated SQL more succinct, these steps will have to be made "smarter". We'll have to add in the math rules to avoid unneeded parentheses and (likely dialect specific) rules for determining of quoting is needed. Note that the latter likely involves listing out the reserved keywords for each dialect. |
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.
Thanks again @edmondop
Thank you for that excellent explanation @backkem . Since this has now come up several times, I filed #10557 to track it |
* Example for simple conversion * Fixing example * Updating README * Update plan_to_sql.rs Update file header with more details about the examples * Fixing formatting * Update README.md Fixing extra -
Which issue does this PR close?
Closes #10524 .
At the time of opening this PR, the example fails with: