Use explicit enum for physical errors #2
Conversation
| @@ -62,6 +62,8 @@ pub enum PlanType { | |||
| FinalPhysicalPlanWithStats, | |||
| /// The final with schema, fully optimized physical plan which would be executed | |||
| FinalPhysicalPlanWithSchema, | |||
| /// An error creating the physical plan | |||
| PhysicalPlanError, | |||
There was a problem hiding this comment.
By adding an explicit new "plan type" it became clear how to show the errors
There was a problem hiding this comment.
Yes, that's what I want to do in the begining, but I didn't want to introduce many overhead so I changed it. Sometimes I am confused about balancing change size and simplicity. Thanks for your suggestion!
There was a problem hiding this comment.
Yeah, it is totally a judgement call
There was a problem hiding this comment.
Yeah, it is totally a judgement call -- I don't think there is ever one right answer
| vec![StringifiedPlan::new(FinalLogicalPlan, err.to_string())], | ||
| e.verbose, | ||
| )))) | ||
| stringified_plans.push(StringifiedPlan::new( |
There was a problem hiding this comment.
this is the core difference -- rather than using a special ExplainExec instead use a new "plan type" that isn't filtered out of the normal explain
| 04)------Cross Join: | ||
| 05)--------TableScan: t1 | ||
| 06)--------TableScan: t2 | ||
| physical_plan_error This feature is not implemented: Unsupported logical plan: Dml(Update) |
There was a problem hiding this comment.
this way we still see the logical explain, but now it is also clear there is a physical plan error
Note this targets apache#13210
While reviewing apache#13210 from @Lordworms I had an alternate suggestion on how to get the errors to appear in the explain plan. This PR implements that suggestion