-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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)
@@ -2637,42 +2827,6 @@ mod tests { | |||
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.
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?
SQLOptions
andSessionContext::sql_with_options
apisAre these changes tested?
Are there any user-facing changes?
New
SQLOptions
andSessionContext::sql_with_options
apisEverything should be backwards compatible