You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/06/04 19:24:17 UTC
[PR] Split `SessionState` into its own module [datafusion]
alamb opened a new pull request, #10794:
URL: https://github.com/apache/datafusion/pull/10794
## Which issue does this PR close?
Part of https://github.com/apache/datafusion/issues/10782
## Rationale for this change
the `datafusion/core/src/execution/context/mod.rs` file is quite large and hard to navigate.
It would be better to have SessionContext and SessionState in their own modules
Also, if we ever are able to break out catalog from the core, e.g. https://github.com/apache/datafusion/issues/10782 we may have to break out `SessionState` as well, so having SessionState in its own module will help here
It also turns out splitting them into their own files makes the boundaries a bit clearer
## What changes are included in this PR?
1. Move `SessionState` out of `datafusion/core/src/execution/context/mod.rs` and into `datafusion/core/src/execution/state.rs`
2. Add a `pub use`
3. Add some additional accessors to `SessionState` to maintain the same behavior
## Are these changes tested?
Exiting CI
## Are there any user-facing changes?
No, this is an internal code reorganization only
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1627961769
##########
datafusion/core/src/execution/state.rs:
##########
Review Comment:
sure!
##########
datafusion/core/src/execution/state.rs:
##########
@@ -0,0 +1,1130 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! [`SessionState`]: information required to run queries in a session
+
+use crate::catalog::information_schema::{InformationSchemaProvider, INFORMATION_SCHEMA};
+use crate::catalog::listing_schema::ListingSchemaProvider;
+use crate::catalog::schema::{MemorySchemaProvider, SchemaProvider};
+use crate::catalog::{
+ CatalogProvider, CatalogProviderList, MemoryCatalogProvider,
+ MemoryCatalogProviderList,
+};
+use crate::datasource::cte_worktable::CteWorkTable;
+use crate::datasource::function::{TableFunction, TableFunctionImpl};
+use crate::datasource::provider::{DefaultTableFactory, TableProviderFactory};
+use crate::datasource::provider_as_source;
+use crate::execution::context::{EmptySerializerRegistry, FunctionFactory, QueryPlanner};
+#[cfg(feature = "array_expressions")]
+use crate::functions_array;
+use crate::physical_optimizer::optimizer::PhysicalOptimizer;
+use crate::physical_optimizer::PhysicalOptimizerRule;
+use crate::physical_planner::{DefaultPhysicalPlanner, PhysicalPlanner};
+use crate::{functions, functions_aggregate};
+use arrow_schema::{DataType, SchemaRef};
+use async_trait::async_trait;
+use chrono::{DateTime, Utc};
+use datafusion_common::alias::AliasGenerator;
+use datafusion_common::config::{ConfigExtension, ConfigOptions, TableOptions};
+use datafusion_common::display::{PlanType, StringifiedPlan, ToStringifiedPlan};
+use datafusion_common::tree_node::TreeNode;
+use datafusion_common::{
+ not_impl_err, plan_datafusion_err, DFSchema, DataFusionError, ResolvedTableReference,
+ TableReference,
+};
+use datafusion_execution::config::SessionConfig;
+use datafusion_execution::object_store::ObjectStoreUrl;
+use datafusion_execution::runtime_env::RuntimeEnv;
+use datafusion_execution::TaskContext;
+use datafusion_expr::execution_props::ExecutionProps;
+use datafusion_expr::expr_rewriter::FunctionRewrite;
+use datafusion_expr::registry::{FunctionRegistry, SerializerRegistry};
+use datafusion_expr::simplify::SimplifyInfo;
+use datafusion_expr::var_provider::{is_system_variables, VarType};
+use datafusion_expr::{
+ AggregateUDF, Explain, Expr, ExprSchemable, LogicalPlan, ScalarUDF, TableSource,
+ WindowUDF,
+};
+use datafusion_optimizer::simplify_expressions::ExprSimplifier;
+use datafusion_optimizer::{
+ Analyzer, AnalyzerRule, Optimizer, OptimizerConfig, OptimizerRule,
+};
+use datafusion_physical_expr::create_physical_expr;
+use datafusion_physical_expr_common::physical_expr::PhysicalExpr;
+use datafusion_physical_plan::ExecutionPlan;
+use datafusion_sql::parser::{CopyToSource, CopyToStatement, DFParser};
+use datafusion_sql::planner::{
+ object_name_to_table_reference, ContextProvider, ParserOptions, SqlToRel,
+};
+use sqlparser::dialect::dialect_from_str;
+use std::collections::hash_map::Entry;
+use std::collections::{HashMap, HashSet};
+use std::fmt::Debug;
+use std::ops::ControlFlow;
+use std::sync::Arc;
+use url::Url;
+use uuid::Uuid;
+
+/// Execution context for registering data sources and executing queries.
Review Comment:
This is just moved from `context/mod.rs`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10794:
URL: https://github.com/apache/datafusion/pull/10794
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1628022118
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1280,7 +1253,7 @@ impl SessionContext {
/// `query_execution_start_time` to the current time
pub fn state(&self) -> SessionState {
let mut state = self.state.read().clone();
- state.execution_props.start_execution();
+ state.execution_props_mut().start_execution();
Review Comment:
It is needed because the start execution method actually modifies the execution_props field -- when SessionContext was part of the same module, it could simply directly modify `state.execution_props` (the field, not a method)
Now that they are in separate modules, SessionContext can't modify `SessionState` internal fields directly.
So TLDR is that this code does the same as the previous version (modifies `execution_props`) but is more explicit about it, which I think is an improvement
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1628018164
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -50,51 +40,37 @@ use crate::{
logical_expr::{
CreateCatalog, CreateCatalogSchema, CreateExternalTable, CreateFunction,
CreateMemoryTable, CreateView, DropCatalogSchema, DropFunction, DropTable,
- DropView, Explain, LogicalPlan, LogicalPlanBuilder, PlanType, SetVariable,
- TableSource, TableType, ToStringifiedPlan, UNNAMED_TABLE,
+ DropView, LogicalPlan, LogicalPlanBuilder, SetVariable, TableType, UNNAMED_TABLE,
},
- optimizer::analyzer::{Analyzer, AnalyzerRule},
- optimizer::optimizer::{Optimizer, OptimizerConfig, OptimizerRule},
- physical_expr::{create_physical_expr, PhysicalExpr},
- physical_optimizer::optimizer::{PhysicalOptimizer, PhysicalOptimizerRule},
+ physical_expr::PhysicalExpr,
physical_plan::ExecutionPlan,
- physical_planner::{DefaultPhysicalPlanner, PhysicalPlanner},
variable::{VarProvider, VarType},
};
-use crate::{functions, functions_aggregate};
-use arrow::datatypes::{DataType, SchemaRef};
+use arrow::datatypes::SchemaRef;
use arrow::record_batch::RecordBatch;
use arrow_schema::Schema;
use datafusion_common::{
- alias::AliasGenerator,
config::{ConfigExtension, TableOptions},
- exec_err, not_impl_err, plan_datafusion_err, plan_err,
- tree_node::{TreeNode, TreeNodeRecursion, TreeNodeVisitor},
+ exec_err, not_impl_err, plan_err,
+ tree_node::{TreeNodeRecursion, TreeNodeVisitor},
DFSchema, SchemaReference, TableReference,
};
use datafusion_execution::registry::SerializerRegistry;
use datafusion_expr::{
expr_rewriter::FunctionRewrite,
logical_plan::{DdlStatement, Statement},
- simplify::SimplifyInfo,
- var_provider::is_system_variables,
- Expr, ExprSchemable, StringifiedPlan, UserDefinedLogicalNode, WindowUDF,
-};
-use datafusion_optimizer::simplify_expressions::ExprSimplifier;
-use datafusion_sql::{
- parser::{CopyToSource, CopyToStatement, DFParser},
- planner::{object_name_to_table_reference, ContextProvider, ParserOptions, SqlToRel},
- ResolvedTableReference,
+ Expr, UserDefinedLogicalNode, WindowUDF,
};
-use sqlparser::dialect::dialect_from_str;
+
+// backwards compatibility
Review Comment:
This `pub use` means there should be no downstream changes required
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#issuecomment-2151986789
Thanks again for the reviews
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1628026437
##########
datafusion/core/src/execution/state.rs:
##########
Review Comment:
in [0c1dbbe](https://github.com/apache/datafusion/pull/10794/commits/0c1dbbe4f14b8c285aad6be579bbe23cc9bda581)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1627972317
##########
datafusion/core/src/execution/context/mod.rs:
##########
@@ -1280,7 +1253,7 @@ impl SessionContext {
/// `query_execution_start_time` to the current time
pub fn state(&self) -> SessionState {
let mut state = self.state.read().clone();
- state.execution_props.start_execution();
+ state.execution_props_mut().start_execution();
Review Comment:
do we need mut here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#discussion_r1627958016
##########
datafusion/core/src/execution/state.rs:
##########
Review Comment:
Can we name it as session_state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Split `SessionState` into its own module [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10794:
URL: https://github.com/apache/datafusion/pull/10794#issuecomment-2150389858
> lgtm thanks @alamb but this might require downstream user changes?
Sorry I should have pointed that out -- I left a `pub use` so I don't think this will require any downstream changes: https://github.com/apache/datafusion/pull/10794/files#r1628018164
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org