Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methods by alamb · Pull Request #4721 · apache/datafusion

alamb · 2022-12-23T19:32:26Z

Which issue does this PR close?

Part of #4617

Rationale for this change

More details on ticket #4720

Basically this moves the planning / execution in DataFusion to be based on DataFrame rather than some sort of mx of SessionContext / SessionState / DataFrame. Started by @tustvold in #4679

What changes are included in this PR?

Change APIs to allow query planning without DDL execution: SessionContext::dataframe_without_ddl
Add doc comments about the usecases
Deprecate SessionContext::optimize and SessionContext::create_physical_plan
Update errors in physical planner to return DataFusion::Error someone attempts to plan a DDL it is unsupported

Are these changes tested?

Yes, existing tests

Are there any user-facing changes?

API is slightly different (different names)
Error messages are more precise

alamb · 2022-12-23T20:49:51Z

 }

- let plan = ctx.optimize(&plan)?;
+ let plan = ctx.dataframe(plan).await?.into_optimized_plan()?;


this is the new pattern to get an optimized plan (rather than calling ctx.optimize directly

alamb · 2022-12-23T20:50:13Z

+ ///
+ /// This method is `async` because queries of type `CREATE
+ /// EXTERNAL TABLE` might require the schema to be inferred.
 pub async fn sql(&self, sql: &str) -> Result<DataFrame> {


the core SessionContext::sql API does not change

alamb · 2022-12-23T20:50:34Z

+ }

+ /// Creates a [`DataFrame`] that will execute the specified
+ /// LogicalPlan, including DDL such as (such as `CREATE TABLE`).


Suggested change
/// LogicalPlan, including DDL such as (such as `CREATE TABLE`).
/// LogicalPlan, including DDL (such as `CREATE TABLE`).

alamb · 2022-12-23T20:51:11Z

+ /// `CREATE TABLE`
+ ///
+ /// Use [`Self::dataframe`] to run plans with DDL
+ pub fn dataframe_without_ddl(&self, plan: LogicalPlan) -> Result<DataFrame> {


This is the API to support #4720

This appears to just be DataFrame new, do we really need this?

alamb · 2022-12-23T20:52:22Z

 // that the appropriate table can be registered with
 // the context)
- Err(DataFusionError::Internal(
+ Err(DataFusionError::Plan(


These are not really "internal errors" as they can be triggered by trying to run a sql query that contains DDL

Imo this is a footgun we should aim to remove, I have a plan

#4721 (comment) are the key usecases -- as long as they are possible / easy / well documented it will be great

tustvold

I'm really not a fan of introducing more complexity into this interface, part of the current mess is from there being 14 different ways to do everything, this appears to be putting back a quirk I'm actively trying to remove.

I think IOx should use the lower-level APIs, such as SessionState and DFParser, longer-term I don't think it should be using the interior mutable SessionContext at all

tustvold · 2022-12-23T23:09:32Z

- pub fn create_logical_plan(&self, sql: &str) -> Result<LogicalPlan> {
+
+ /// Creates a [`LogicalPlan`] from a SQL query.
+ pub fn plan_sql(&self, sql: &str) -> Result<LogicalPlan> {


Again this appears to just call DFParser followed by the query planner

It is also problematic for the same reason that create_logical_plan is problematic - it returns a LogicalPlan without any mechanism to optimise/execute against the same state

Again this appears to just call DFParser followed by the query planner
Yes that is exactly what it does.
There needs to be some way for users to create a LogicalPlan and get datafusion to optimize and run it properly it (e.g. if the user makes the LogicalPlan directly from their own query language such as influxrpc or VegaFusion)

alamb · 2022-12-24T12:29:26Z

I think some key capabilities for all users of DataFusion (including IOx) are:

Create a LogicalPlan from some source (e.g your own frontend) and have datafusion plan / run that for you
Read only SQL (no DDL / session state modifications via SQL)

As long as those are possible and well documented I do not have strong opinions on the API

alamb · 2022-12-24T12:33:34Z

Marking as a Draft as I think @tustvold plans an alternate proposal and we can continue to us the deprecated APIs in IOx for the time being

tustvold · 2022-12-24T12:35:51Z

I will work on this after christmas (27th). I think the key thing is to not be mixing high-level APIs i.e. SessionContext with low-level concepts LogicalPlan. The key thing is to ensure that planning, optimisation and execution take place against the same SessionState. The interior mutability of SessionContext makes this impossible

tustvold · 2022-12-27T11:41:07Z

POC in https://github.com/influxdata/influxdb_iox/pull/6469 - the TLDR is to use SessionState directly

alamb · 2022-12-28T12:23:15Z

This PR has been rendered obsolete via #4750

github-actionsBot added the core Core DataFusion crate label Dec 23, 2022

alamb changed the title ~~Non-deprecated support for planning SQL without DDL~~Non-deprecated support for planning SQL without DDL, deprecate some more SessionContext methodsDec 23, 2022

alamb force-pushed the alamb/clarify_interface branch from beef643 to e733070Compare December 23, 2022 20:47

alamb added the api change Changes the API exposed to users of the crate label Dec 23, 2022

alamb force-pushed the alamb/clarify_interface branch from e733070 to c6291ffCompare December 23, 2022 20:55

alamb commented Dec 23, 2022
View reviewed changes

alamb requested a review from tustvold December 23, 2022 20:56

alamb force-pushed the alamb/clarify_interface branch from c6291ff to 052caebCompare December 23, 2022 21:14

Non-deprecated support for planning SQL without DDL
b9ff071

alamb force-pushed the alamb/clarify_interface branch from 052caeb to b9ff071Compare December 23, 2022 21:45

alamb marked this pull request as ready for review December 23, 2022 22:48

alamb requested a review from andygrove December 23, 2022 22:48

tustvold reviewed Dec 23, 2022
View reviewed changes

alamb marked this pull request as draft December 24, 2022 12:33

alamb closed this Dec 28, 2022

alamb deleted the alamb/clarify_interface branch August 8, 2023 20:14

UlfarErl mentioned this pull request Aug 12, 2023
Misleading/invalid documentation on SessionContext::sql and SessionConfig::create_logical_plan #7272
Closed

	/// LogicalPlan, including DDL such as (such as `CREATE TABLE`).
	/// LogicalPlan, including DDL (such as `CREATE TABLE`).

Uh oh!

Conversation

alamb commented Dec 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvoldDec 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 24, 2022

Uh oh!

alamb commented Dec 24, 2022

Uh oh!

tustvold commented Dec 24, 2022

Uh oh!

tustvold commented Dec 27, 2022

Uh oh!

alamb commented Dec 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alamb commented Dec 23, 2022•
edited
Loading

tustvoldDec 23, 2022•
edited
Loading

tustvold left a comment •
edited
Loading