-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Move SQL Query Planner into new datafusion-sql crate
#2588
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
Conversation
| quick_test(sql, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_offset_after_limit_with_limit_push() { |
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 shouldn't be testing optimizer rules here, just the SQL query planner
datafusion-sql cratedatafusion-sql crate
| https://crates.io/crates/datafusion-physical-expr/8.0.0 | ||
| https://crates.io/crates/datafusion-proto/8.0.0 | ||
| https://crates.io/crates/datafusion-data-access/8.0.0 | ||
| https://crates.io/crates/ballista/0.7.0 |
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.
removing ballista here is not related to this PR but was missed earlier
yjshen
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 didn't review the code of this PR but I like where it is heading -- thank you @andygrove |
Which issue does this PR close?
Closes #2345
Rationale for this change
I would like the ability to have projects just depend on datafusion-sql if they are using DataFusion as a SQL parser + query planner and are not using it for execution.
What changes are included in this PR?
Here is the new crate dependency diagram:
Are there any user-facing changes?
Yes, API change and new crate.