let PhysicalOptimizerRule accept SessionConfig instead of ConfigOptions

[gabotechs]

Which issue does this PR close?

• No issue.

Rationale for this change

Users might want to create their own PhysicalOptimizerRule implementations. Users might have their own SessionConfig.extensions. Users might want to use their own SessionConfig.extensions in their custom PhysicalOptimizerRule implementations.

This PR is needed for gabotechs#7, but it was factored out as it's an isolated change that could have value on its own.

What changes are included in this PR?

Changes signature of PhysicalOptimizerRule.optimze to take a SessionConfig rather than a ConfigOptions.

pub trait PhysicalOptimizerRule: Debug {
    /// Rewrite `plan` to an optimized form
    fn optimize(
        &self,
        plan: Arc<dyn ExecutionPlan>,
-       config: &ConfigOptions,
+       config: &SessionConfig,
    ) -> Result<Arc<dyn ExecutionPlan>>;
    ...
}

Are these changes tested?

yes, by current tests.

Are there any user-facing changes?

yes, PhysicalOptimizerRule.optimze now takes a SessionConfig rather than a ConfigOptions, which is a breaking non backwards compatible change.

[adriangb]

This seems like a reasonable change to me, but I think we should wait until we are certain we are going to need it for #18172 before we merge it to avoid breaking the API without good justification

[adriangb]

@gabotechs sorry for not coming back to this. I’d like to help get it across the line. Are you okay if I resolve conflicts and push?

[adriangb]

My only feedback for review is if we should add a new argument with Extensions or make the argument OptimizerContext so that we can make future changes without it being breaking

[gabotechs]

@gabotechs sorry for not coming back to this. I’d like to help get it across the line. Are you okay if I resolve conflicts and push?

Sure!

[gabotechs]

My only feedback for review is if we should add a new argument with Extensions or make the argument OptimizerContext so that we can make future changes without it being breaking

I'm trying to look for inspiration in the docs for this, and I found:
https://docs.rs/datafusion/latest/datafusion/execution/context/struct.SessionContext.html#relationship-between-sessioncontext-sessionstate-and-taskcontext

Relationship between SessionContext, SessionState, and TaskContext

The state required to optimize, and evaluate queries is broken into three levels to allow tailoring

The objects are:

SessionContext: Most users should use a SessionContext. It contains all information required to execute queries including high level APIs such as SessionContext::sql. All queries run with the same SessionContext share the same configuration and resources (e.g. memory limits).

SessionState: contains information required to plan and execute an individual query (e.g. creating a LogicalPlan or ExecutionPlan). Each query is planned and executed using its own SessionState, which can be created with SessionContext::state. SessionState allows finer grained control over query execution, for example disallowing DDL operations such as CREATE TABLE.

TaskContext contains the state required for query execution (e.g. ExecutionPlan::execute). It contains a subset of information in SessionState. TaskContext allows executing ExecutionPlans PhysicalExprs without requiring a full SessionState.

Following that same pattern, for the same reason that a TaskContext exists for execution, one could argue that an equivalent OptimizerContext struct for optimization steps could make sense.

WDYT?

[gabotechs]

Also, found another different reason for wanting this:

https://github.com/datafusion-contrib/datafusion-distributed/blob/main/src/distributed_planner/distributed_config.rs#L87-L94

[adriangb]

Following that same pattern, for the same reason that a TaskContext exists for execution, one could argue that an equivalent OptimizerContext struct for optimization steps could make sense.

Yep agreed! And we can even add a new method to the trait optimize_plan or something that accepts an OptimizerContext and delegates to the existing method by default to ease the transition / make it a non breaking change for now. It will still be tricky to deprecate the original method, there is no deprecation of trait methods…

[adriangb]

Something like #18739

[gabotechs]

Closing in favor of #18739