chore: public sql_statement_to_plan_with_context() #7268
Conversation
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @waynexia
This looks fine to me -- if you plan to use this as an external project, I recommend you add some sort of end to end test (perhaps somewhere in https://github.com/apache/arrow-datafusion/tree/main/datafusion-examples/examples) -- otherwise I could see this being easily broken in a refactor
|
|
||
| /// Generate a logical plan from an SQL statement | ||
| fn sql_statement_to_plan_with_context( | ||
| pub fn sql_statement_to_plan_with_context( |
There was a problem hiding this comment.
Could you rename sql_statement_to_plan_with_context_impl to sql_statement_to_plan_with_context and make it pub and get the same effect with less code?
There was a problem hiding this comment.
maybe the author didn't want to change the api? and only want to make it public?
There was a problem hiding this comment.
👍 I think this PR could be merged in as is. @waynexia feel free to merge it (or let us know you plan to make more changes)
|
Merging to reduce the number of outstanding pull requests. Thanks everyone for your comments |
3 participants
Which issue does this PR close?
Closes #.
Rationale for this change
This patch makes
sql_statement_to_plan_with_context()public. Compared tosql_statement_to_plan, it accepts an externalPlannerContext, which allows pre-registering some external context for planningWhat changes are included in this PR?
Like above
Are these changes tested?
n/a
Are there any user-facing changes?
A new public API
sql_statement_to_plan_with_context