Skip to content
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

Misleading/invalid documentation on SessionContext::sql and SessionConfig::create_logical_plan #7272

Closed
UlfarErl opened this issue Aug 12, 2023 · 3 comments · Fixed by #7333
Assignees

Comments

@UlfarErl
Copy link

The comments for these two functions refer to each other, and the comment for SessionContext::sql indicates that create_logical_plan can be used to avoid DDL / DML statements etc. However, create_logical_plan doesn't restrict such statements (in particular, it is used by SessionContext::sql itself).

https://github.com/apache/arrow-datafusion/blob/00627785718d9d98998021bf44585f32c33af3ea/datafusion/core/src/execution/context.rs#L351C11-L351C11
https://github.com/apache/arrow-datafusion/blob/00627785718d9d98998021bf44585f32c33af3ea/datafusion/core/src/execution/context.rs#L1777

@UlfarErl
Copy link
Author

@alamb I spotted this when looking for a read-only LogicalPlan interface, just like the one you were aiming for in issue #4720 (and your PR #4721). Those were closed without any apparent plan for such an interface. It would be great to document (even if planned for the future).

@alamb
Copy link
Contributor

alamb commented Aug 13, 2023

Thanks @UlfarErl -- I agree the comments and API could be improved -- I'll try and improve them this week

The key for "read only" is to not call SessionContext::execute_logical_plan -- so do

SessionContext::create_logical_plan

And then

session_context.state().create_physical_plan

I agree this is not clear from the docs.

@alamb
Copy link
Contributor

alamb commented Aug 18, 2023

Hi @UlfarErl -- I made a PR to update the documentation here #7327

While working on it, it became clear the API is somewhat confusing (as you have already pointed out) #7328 -- PR to fix: #7333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants