Skip to content

Commit

Permalink
Allow skip_failed_rules to skip buggy logical plan rules that have …
Browse files Browse the repository at this point in the history
…a schema mismatch (apache#7277)

* allow skipping buggy logical plan rules

* test skipping failed rule

* improve error chaining

* fix: clippy

---------

Co-authored-by: Andrew Lamb <[email protected]>
  • Loading branch information
smiklos and alamb authored Aug 14, 2023
1 parent 2ec0bc1 commit e253b8e
Showing 1 changed file with 22 additions and 7 deletions.
29 changes: 22 additions & 7 deletions datafusion/optimizer/src/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,16 @@ impl Optimizer {
log_plan(&format!("Optimizer input (pass {i})"), &new_plan);

for rule in &self.rules {
let result = self.optimize_recursively(rule, &new_plan, config);

let result =
self.optimize_recursively(rule, &new_plan, config)
.and_then(|plan| {
if let Some(plan) = &plan {
assert_schema_is_the_same(rule.name(), &new_plan, plan)?;
}
Ok(plan)
});
match result {
Ok(Some(plan)) => {
assert_schema_is_the_same(rule.name(), &new_plan, &plan)?;
new_plan = plan;
observer(&new_plan, rule.as_ref());
log_plan(rule.name(), &new_plan);
Expand Down Expand Up @@ -428,8 +433,7 @@ fn assert_schema_is_the_same(

if !equivalent {
let e = DataFusionError::Internal(format!(
"Optimizer rule '{}' failed, due to generate a different schema, original schema: {:?}, new schema: {:?}",
rule_name,
"Failed due to generate a different schema, original schema: {:?}, new schema: {:?}",
prev_plan.schema(),
new_plan.schema()
));
Expand Down Expand Up @@ -493,8 +497,8 @@ mod tests {
});
let err = opt.optimize(&plan, &config, &observe).unwrap_err();
assert_eq!(
"get table_scan rule\ncaused by\n\
Internal error: Optimizer rule 'get table_scan rule' failed, due to generate a different schema, \
"Optimizer rule 'get table_scan rule' failed\ncaused by\nget table_scan rule\ncaused by\n\
Internal error: Failed due to generate a different schema, \
original schema: DFSchema { fields: [], metadata: {}, functional_dependencies: FunctionalDependencies { deps: [] } }, \
new schema: DFSchema { fields: [\
DFField { qualifier: Some(Bare { table: \"test\" }), field: Field { name: \"a\", data_type: UInt32, nullable: false, dict_id: 0, dict_is_ordered: false, metadata: {} } }, \
Expand All @@ -507,6 +511,17 @@ mod tests {
);
}

#[test]
fn skip_generate_different_schema() {
let opt = Optimizer::with_rules(vec![Arc::new(GetTableScanRule {})]);
let config = OptimizerContext::new().with_skip_failing_rules(true);
let plan = LogicalPlan::EmptyRelation(EmptyRelation {
produce_one_row: false,
schema: Arc::new(DFSchema::empty()),
});
opt.optimize(&plan, &config, &observe).unwrap();
}

#[test]
fn generate_same_schema_different_metadata() -> Result<()> {
// if the plan creates more metadata than previously (because
Expand Down

0 comments on commit e253b8e

Please sign in to comment.