From 7bd43758895f86033032979f8833ba8dc05b0591 Mon Sep 17 00:00:00 2001 From: Chongchen Chen Date: Mon, 19 May 2025 18:45:34 +0800 Subject: [PATCH 1/4] feat: make error handling in indent consistent with that in tree --- datafusion/core/src/physical_planner.rs | 50 ++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index be24206c676c..34d26c6b0728 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -1757,6 +1757,12 @@ impl DefaultPhysicalPlanner { ))); } + if !e.logical_optimization_succeeded { + if let Some(plan) = e.stringified_plans.last() { + return exec_err!("{}", plan.plan); + } + } + // The indent mode is quite sophisticated, and handles quite a few // different cases / options for displaying the plan. if !config.physical_plan_only { @@ -2192,7 +2198,9 @@ mod tests { use arrow::array::{ArrayRef, DictionaryArray, Int32Array}; use arrow::datatypes::{DataType, Field, Int32Type}; use datafusion_common::config::ConfigOptions; - use datafusion_common::{assert_contains, DFSchemaRef, TableReference}; + use datafusion_common::{ + assert_contains, DFSchemaRef, TableReference, ToDFSchema as _, + }; use datafusion_execution::runtime_env::RuntimeEnv; use datafusion_execution::TaskContext; use datafusion_expr::{col, lit, LogicalPlanBuilder, UserDefinedLogicalNodeCore}; @@ -2689,6 +2697,46 @@ mod tests { } } + #[tokio::test] + async fn test_explain_indent_err() { + let planner = DefaultPhysicalPlanner::default(); + let ctx = SessionContext::new(); + let schema = Schema::new(vec![Field::new("id", DataType::Int32, false)]); + let plan = Arc::new( + scan_empty(Some("employee"), &schema, None) + .unwrap() + .explain(true, false) + .unwrap() + .build() + .unwrap(), + ); + + // Create a schema + let schema = Arc::new(Schema::new(vec![ + Field::new("plan_type", DataType::Utf8, false), + Field::new("plan", DataType::Utf8, false), + ])); + + // Create invalid indentation in the plan + let stringified_plans = + vec![StringifiedPlan::new(PlanType::FinalLogicalPlan, "Test Err")]; + + let explain = Explain { + verbose: false, + explain_format: ExplainFormat::Indent, + plan, + stringified_plans, + schema: schema.to_dfschema_ref().unwrap(), + logical_optimization_succeeded: false, + }; + let result = planner.handle_explain(&explain, &ctx.state()).await; + + // Verify that the execution fails due to indentation error + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_eq!(err.to_string(), "Execution error: Test Err"); + } + #[tokio::test] async fn test_maybe_fix_colon_in_physical_name() { // The physical schema has a field name with a colon From eea7b513a2ce03f79241035e989f8b1d37b8656f Mon Sep 17 00:00:00 2001 From: Chongchen Chen Date: Mon, 19 May 2025 20:59:28 +0800 Subject: [PATCH 2/4] update test --- datafusion/core/src/physical_planner.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index 34d26c6b0728..9a9c39c6826b 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -2734,7 +2734,7 @@ mod tests { // Verify that the execution fails due to indentation error assert!(result.is_err()); let err = result.unwrap_err(); - assert_eq!(err.to_string(), "Execution error: Test Err"); + assert!(err.to_string().starts_with("Execution error: Test Err")); } #[tokio::test] From b845c7d9726de198518f4f6ddef7c6f69a0c4d6c Mon Sep 17 00:00:00 2001 From: Chongchen Chen Date: Tue, 20 May 2025 08:21:08 +0800 Subject: [PATCH 3/4] return all plans instead of throwing err --- datafusion/core/src/physical_planner.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index 9a9c39c6826b..d7c633145265 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -1714,6 +1714,14 @@ impl DefaultPhysicalPlanner { let config = &session_state.config_options().explain; let explain_format = &e.explain_format; + if !e.logical_optimization_succeeded { + return Ok(Arc::new(ExplainExec::new( + Arc::clone(e.schema.inner()), + e.stringified_plans.clone(), + e.verbose, + ))); + } + match explain_format { ExplainFormat::Indent => { /* fall through */ } ExplainFormat::Tree => { @@ -1757,12 +1765,6 @@ impl DefaultPhysicalPlanner { ))); } - if !e.logical_optimization_succeeded { - if let Some(plan) = e.stringified_plans.last() { - return exec_err!("{}", plan.plan); - } - } - // The indent mode is quite sophisticated, and handles quite a few // different cases / options for displaying the plan. if !config.physical_plan_only { From 81410a64ebd11c5ebd2bb519f55e4c14211926bc Mon Sep 17 00:00:00 2001 From: Chongchen Chen Date: Tue, 20 May 2025 08:43:15 +0800 Subject: [PATCH 4/4] update test --- datafusion/core/src/physical_planner.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index d7c633145265..7313c5ce79b7 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -1718,7 +1718,7 @@ impl DefaultPhysicalPlanner { return Ok(Arc::new(ExplainExec::new( Arc::clone(e.schema.inner()), e.stringified_plans.clone(), - e.verbose, + true, ))); } @@ -2731,12 +2731,20 @@ mod tests { schema: schema.to_dfschema_ref().unwrap(), logical_optimization_succeeded: false, }; - let result = planner.handle_explain(&explain, &ctx.state()).await; - - // Verify that the execution fails due to indentation error - assert!(result.is_err()); - let err = result.unwrap_err(); - assert!(err.to_string().starts_with("Execution error: Test Err")); + let plan = planner + .handle_explain(&explain, &ctx.state()) + .await + .unwrap(); + if let Some(plan) = plan.as_any().downcast_ref::() { + let stringified_plans = plan.stringified_plans(); + assert_eq!(stringified_plans.len(), 1); + assert_eq!(stringified_plans[0].plan.as_str(), "Test Err"); + } else { + panic!( + "Plan was not an explain plan: {}", + displayable(plan.as_ref()).indent(true) + ); + } } #[tokio::test]