-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add SQLOptions for controlling allowed SQL statements, update docs
#7333
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
| /// let results = df.collect(); | ||
| /// let results = df | ||
| /// .collect() | ||
| /// .await?; |
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.
since the results aren't checked it turns out this code was not actually running the plan (missing an await so I fixed that)
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::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.
I moved this into core_integration as it seemed like a test of the public API rather than an internal unit test
| LogicalPlan::Dml(dml) if !self.options.allow_dml => { | ||
| plan_err!("DML not supported: {}", dml.op) | ||
| } | ||
| LogicalPlan::Copy(_) if !self.options.allow_dml => { |
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 so much @alamb for adding this to the core DF APIs. The addition of the Copy operator is a great example of why it belongs here: we already have a visitor that does similar filtering, but we would probably have failed to update it to include Copy when we migrate to the latest DF version.
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.
The reasons stated in the issue sound reasonable, and the new API is neat! Thanks @alamb!
Which issue does this PR close?
Closes #7328
Closes #7272
Rationale for this change
See #7328
Basically it is hard to understand how to control what SQL statements DataFusion will allow
What changes are included in this PR?
SQLOptionsandSessionContext::sql_with_optionsapisAre these changes tested?
Are there any user-facing changes?
New
SQLOptionsandSessionContext::sql_with_optionsapisEverything should be backwards compatible