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

Derive Debug for SessionStateBuilder, adding Debug requirements to fields #12632

Merged
merged 16 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions datafusion-cli/src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use dirs::home_dir;
use parking_lot::RwLock;

/// Wraps another catalog, automatically register require object stores for the file locations
#[derive(Debug)]
pub struct DynamicObjectStoreCatalog {
inner: Arc<dyn CatalogProviderList>,
state: Weak<RwLock<SessionState>>,
Expand Down Expand Up @@ -74,6 +75,7 @@ impl CatalogProviderList for DynamicObjectStoreCatalog {
}

/// Wraps another catalog provider
#[derive(Debug)]
struct DynamicObjectStoreCatalogProvider {
inner: Arc<dyn CatalogProvider>,
state: Weak<RwLock<SessionState>>,
Expand Down Expand Up @@ -115,6 +117,7 @@ impl CatalogProvider for DynamicObjectStoreCatalogProvider {

/// Wraps another schema provider. [DynamicObjectStoreSchemaProvider] is responsible for registering the required
/// object stores for the file locations.
#[derive(Debug)]
struct DynamicObjectStoreSchemaProvider {
inner: Arc<dyn SchemaProvider>,
state: Weak<RwLock<SessionState>>,
Expand Down
1 change: 1 addition & 0 deletions datafusion-cli/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ fn fixed_len_byte_array_to_string(val: Option<&FixedLenByteArray>) -> Option<Str
})
}

#[derive(Debug)]
pub struct ParquetMetadataFunc {}

impl TableFunctionImpl for ParquetMetadataFunc {
Expand Down
3 changes: 3 additions & 0 deletions datafusion-examples/examples/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ struct DirSchemaOpts<'a> {
format: Arc<dyn FileFormat>,
}
/// Schema where every file with extension `ext` in a given `dir` is a table.
#[derive(Debug)]
struct DirSchema {
ext: String,
tables: RwLock<HashMap<String, Arc<dyn TableProvider>>>,
Expand Down Expand Up @@ -218,6 +219,7 @@ impl SchemaProvider for DirSchema {
}
}
/// Catalog holds multiple schemas
#[derive(Debug)]
struct DirCatalog {
schemas: RwLock<HashMap<String, Arc<dyn SchemaProvider>>>,
}
Expand Down Expand Up @@ -259,6 +261,7 @@ impl CatalogProvider for DirCatalog {
}
}
/// Catalog lists holds multiple catalog providers. Each context has a single catalog list.
#[derive(Debug)]
struct CustomCatalogProviderList {
catalogs: RwLock<HashMap<String, Arc<dyn CatalogProvider>>>,
}
Expand Down
1 change: 1 addition & 0 deletions datafusion-examples/examples/simple_udtf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl TableProvider for LocalCsvTable {
}
}

#[derive(Debug)]
struct LocalCsvTableFunc {}

impl TableFunctionImpl for LocalCsvTableFunc {
Expand Down
5 changes: 3 additions & 2 deletions datafusion/catalog/src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
// under the License.

use std::any::Any;
use std::fmt::Debug;
use std::sync::Arc;

pub use crate::schema::SchemaProvider;
Expand Down Expand Up @@ -101,7 +102,7 @@ use datafusion_common::Result;
///
/// [`TableProvider`]: crate::TableProvider

pub trait CatalogProvider: Sync + Send {
pub trait CatalogProvider: Debug + Sync + Send {
/// Returns the catalog provider as [`Any`]
/// so that it can be downcast to a specific implementation.
fn as_any(&self) -> &dyn Any;
Expand Down Expand Up @@ -152,7 +153,7 @@ pub trait CatalogProvider: Sync + Send {
///
/// Please see the documentation on `CatalogProvider` for details of
/// implementing a custom catalog.
pub trait CatalogProviderList: Sync + Send {
pub trait CatalogProviderList: Debug + Sync + Send {
/// Returns the catalog list as [`Any`]
/// so that it can be downcast to a specific implementation.
fn as_any(&self) -> &dyn Any;
Expand Down
6 changes: 5 additions & 1 deletion datafusion/catalog/src/dynamic_file/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
use crate::{CatalogProvider, CatalogProviderList, SchemaProvider, TableProvider};
use async_trait::async_trait;
use std::any::Any;
use std::fmt::Debug;
use std::sync::Arc;

/// Wrap another catalog provider list
#[derive(Debug)]
pub struct DynamicFileCatalog {
/// The inner catalog provider list
inner: Arc<dyn CatalogProviderList>,
Expand Down Expand Up @@ -67,6 +69,7 @@ impl CatalogProviderList for DynamicFileCatalog {
}

/// Wraps another catalog provider
#[derive(Debug)]
struct DynamicFileCatalogProvider {
/// The inner catalog provider
inner: Arc<dyn CatalogProvider>,
Expand Down Expand Up @@ -114,6 +117,7 @@ impl CatalogProvider for DynamicFileCatalogProvider {
///
/// The provider will try to create a table provider from the file path if the table provider
/// isn't exist in the inner schema provider.
#[derive(Debug)]
pub struct DynamicFileSchemaProvider {
/// The inner schema provider
inner: Arc<dyn SchemaProvider>,
Expand Down Expand Up @@ -174,7 +178,7 @@ impl SchemaProvider for DynamicFileSchemaProvider {

/// [UrlTableFactory] is a factory that can create a table provider from the given url.
#[async_trait]
pub trait UrlTableFactory: Sync + Send {
pub trait UrlTableFactory: Debug + Sync + Send {
/// create a new table provider from the provided url
async fn try_new(
&self,
Expand Down
3 changes: 2 additions & 1 deletion datafusion/catalog/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
use async_trait::async_trait;
use datafusion_common::{exec_err, DataFusionError};
use std::any::Any;
use std::fmt::Debug;
use std::sync::Arc;

use crate::table::TableProvider;
Expand All @@ -32,7 +33,7 @@ use datafusion_common::Result;
///
/// [`CatalogProvider`]: super::CatalogProvider
#[async_trait]
pub trait SchemaProvider: Sync + Send {
pub trait SchemaProvider: Debug + Sync + Send {
/// Returns the owner of the Schema, default is None. This value is reported
/// as part of `information_tables.schemata
fn owner_name(&self) -> Option<&str> {
Expand Down
1 change: 1 addition & 0 deletions datafusion/catalog/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ impl From<&dyn Session> for TaskContext {
}
type SessionRefLock = Arc<Mutex<Option<Weak<RwLock<dyn Session>>>>>;
/// The state store that stores the reference of the runtime session state.
#[derive(Debug)]
pub struct SessionStore {
session: SessionRefLock,
}
Expand Down
14 changes: 3 additions & 11 deletions datafusion/core/src/catalog_common/information_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use arrow::{
};
use async_trait::async_trait;
use datafusion_common::DataFusionError;
use std::fmt::{Debug, Formatter};
use std::fmt::Debug;
use std::{any::Any, sync::Arc};

use crate::catalog::{CatalogProviderList, SchemaProvider, TableProvider};
Expand Down Expand Up @@ -57,6 +57,7 @@ pub const INFORMATION_SCHEMA_TABLES: &[&str] =
/// demand. This means that if more tables are added to the underlying
/// providers, they will appear the next time the `information_schema`
/// table is queried.
#[derive(Debug)]
pub struct InformationSchemaProvider {
config: InformationSchemaConfig,
}
Expand All @@ -70,20 +71,11 @@ impl InformationSchemaProvider {
}
}

#[derive(Clone)]
#[derive(Clone, Debug)]
struct InformationSchemaConfig {
catalog_list: Arc<dyn CatalogProviderList>,
}

impl Debug for InformationSchemaConfig {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
f.debug_struct("InformationSchemaConfig")
// TODO it would be great to print the catalog list here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

// but that would require CatalogProviderList to implement Debug
.finish_non_exhaustive()
}
}

impl InformationSchemaConfig {
/// Construct the `information_schema.tables` virtual table
async fn make_tables(
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/src/catalog_common/listing_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ use object_store::ObjectStore;
/// - `s3://host.example.com:3000/data/tpch/customer/_delta_log/`
///
/// [`ObjectStore`]: object_store::ObjectStore
#[derive(Debug)]
pub struct ListingSchemaProvider {
authority: String,
path: object_store::path::Path,
Expand Down
4 changes: 4 additions & 0 deletions datafusion/core/src/catalog_common/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use std::any::Any;
use std::sync::Arc;

/// Simple in-memory list of catalogs
#[derive(Debug)]
pub struct MemoryCatalogProviderList {
/// Collection of catalogs containing schemas and ultimately TableProviders
pub catalogs: DashMap<String, Arc<dyn CatalogProvider>>,
Expand Down Expand Up @@ -71,6 +72,7 @@ impl CatalogProviderList for MemoryCatalogProviderList {
}

/// Simple in-memory implementation of a catalog.
#[derive(Debug)]
pub struct MemoryCatalogProvider {
schemas: DashMap<String, Arc<dyn SchemaProvider>>,
}
Expand Down Expand Up @@ -136,6 +138,7 @@ impl CatalogProvider for MemoryCatalogProvider {
}

/// Simple in-memory implementation of a schema.
#[derive(Debug)]
pub struct MemorySchemaProvider {
tables: DashMap<String, Arc<dyn TableProvider>>,
}
Expand Down Expand Up @@ -248,6 +251,7 @@ mod test {
#[test]
fn default_register_schema_not_supported() {
// mimic a new CatalogProvider and ensure it does not support registering schemas
#[derive(Debug)]
struct TestProvider {}
impl CatalogProvider for TestProvider {
fn as_any(&self) -> &dyn Any {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/core/src/datasource/dynamic_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::error::Result;
use crate::execution::context::SessionState;

/// [DynamicListTableFactory] is a factory that can create a [ListingTable] from the given url.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct DynamicListTableFactory {
/// The session store that contains the current session.
session_store: SessionStore,
Expand Down
4 changes: 3 additions & 1 deletion datafusion/core/src/datasource/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,17 @@ use super::TableProvider;
use datafusion_common::Result;
use datafusion_expr::Expr;

use std::fmt::Debug;
use std::sync::Arc;

/// A trait for table function implementations
pub trait TableFunctionImpl: Sync + Send {
pub trait TableFunctionImpl: Debug + Sync + Send {
/// Create a table provider
fn call(&self, args: &[Expr]) -> Result<Arc<dyn TableProvider>>;
}

/// A table that uses a function to generate data
#[derive(Debug)]
pub struct TableFunction {
/// Name of the table function
name: String,
Expand Down
6 changes: 4 additions & 2 deletions datafusion/core/src/execution/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1550,7 +1550,7 @@ impl From<SessionContext> for SessionStateBuilder {

/// A planner used to add extensions to DataFusion logical and physical plans.
#[async_trait]
pub trait QueryPlanner {
pub trait QueryPlanner: Debug {
/// Given a `LogicalPlan`, create an [`ExecutionPlan`] suitable for execution
async fn create_physical_plan(
&self,
Expand All @@ -1563,7 +1563,7 @@ pub trait QueryPlanner {
/// and interact with [SessionState] to registers new udf, udaf or udwf.

#[async_trait]
pub trait FunctionFactory: Sync + Send {
pub trait FunctionFactory: Debug + Sync + Send {
/// Handles creation of user defined function specified in [CreateFunction] statement
async fn create(
&self,
Expand All @@ -1586,6 +1586,7 @@ pub enum RegisterFunction {

/// Default implementation of [SerializerRegistry] that throws unimplemented error
/// for all requests.
#[derive(Debug)]
pub struct EmptySerializerRegistry;

impl SerializerRegistry for EmptySerializerRegistry {
Expand Down Expand Up @@ -2132,6 +2133,7 @@ mod tests {
}
}

#[derive(Debug)]
struct MyQueryPlanner {}

#[async_trait]
Expand Down
52 changes: 42 additions & 10 deletions datafusion/core/src/execution/session_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,23 +177,23 @@ impl Debug for SessionState {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("SessionState")
.field("session_id", &self.session_id)
.field("analyzer", &"...")
.field("config", &self.config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could simply #derive(Debug) for SessionState now?

And perhaps we could provide a impl Display for SessionState that had a more readable format (like that shows session_id and deviation from default configuration, for example 🤔 )

.field("runtime_env", &self.runtime_env)
.field("catalog_list", &"...")
.field("serializer_registry", &"...")
.field("execution_props", &self.execution_props)
.field("table_options", &self.table_options)
.field("table_factories", &"...")
.field("function_factory", &"...")
.field("expr_planners", &"...")
.field("query_planner", &"...")
.field("analyzer", &"...")
.field("optimizer", &"...")
.field("physical_optimizers", &"...")
.field("query_planner", &"...")
.field("catalog_list", &"...")
.field("table_functions", &"...")
.field("scalar_functions", &self.scalar_functions)
.field("aggregate_functions", &self.aggregate_functions)
.field("window_functions", &self.window_functions)
.field("serializer_registry", &"...")
.field("config", &self.config)
.field("table_options", &self.table_options)
.field("execution_props", &self.execution_props)
.field("table_factories", &"...")
.field("runtime_env", &self.runtime_env)
.field("function_factory", &"...")
.finish_non_exhaustive()
}
}
Expand Down Expand Up @@ -1519,6 +1519,37 @@ impl SessionStateBuilder {
}
}

impl Debug for SessionStateBuilder {
/// Prefer having short fields at the top and long vector fields near the end
/// Group fields by
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("SessionStateBuilder")
.field("session_id", &self.session_id)
.field("config", &self.config)
.field("runtime_env", &self.runtime_env)
.field("catalog_list", &self.catalog_list)
.field("serializer_registry", &self.serializer_registry)
.field("file_formats", &self.file_formats)
.field("execution_props", &self.execution_props)
.field("table_options", &self.table_options)
.field("table_factories", &self.table_factories)
.field("function_factory", &self.function_factory)
.field("expr_planners", &self.expr_planners)
.field("query_planners", &self.query_planner)
.field("analyzer_rules", &self.analyzer_rules)
.field("analyzer", &self.analyzer)
.field("optimizer_rules", &self.optimizer_rules)
.field("optimizer", &self.optimizer)
.field("physical_optimizer_rules", &self.physical_optimizer_rules)
.field("physical_optimizers", &self.physical_optimizers)
.field("table_functions", &self.table_functions)
.field("scalar_functions", &self.scalar_functions)
.field("aggregate_functions", &self.aggregate_functions)
.field("window_functions", &self.window_functions)
.finish()
}
}

impl Default for SessionStateBuilder {
fn default() -> Self {
Self::new()
Expand Down Expand Up @@ -1795,6 +1826,7 @@ impl From<&SessionState> for TaskContext {
}

/// The query planner used if no user defined planner is provided
#[derive(Debug)]
struct DefaultQueryPlanner {}

#[async_trait]
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/tests/user_defined/expr_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use datafusion_expr::expr::Alias;
use datafusion_expr::planner::{ExprPlanner, PlannerResult, RawBinaryExpr};
use datafusion_expr::BinaryExpr;

#[derive(Debug)]
struct MyCustomPlanner;

impl ExprPlanner for MyCustomPlanner {
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/tests/user_defined/user_defined_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ fn make_topk_context() -> SessionContext {

// ------ The implementation of the TopK code follows -----

#[derive(Debug)]
struct TopKQueryPlanner {}

#[async_trait]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ impl SimpleCsvTable {
}
}

#[derive(Debug)]
struct SimpleCsvTableFunc {}

impl TableFunctionImpl for SimpleCsvTableFunc {
Expand Down
Loading