after type coercion CommonSubexprEliminate will produce invalid projection
#3635
Comments
|
cc @andygrove |
|
Do you have time to take a look this issue? |
|
I was able to hunt down the cause of the bug, I think: In Accordingly, I don't think the issue is limited to This can be seen in the following test (place this in the same file to reproduce): #[test]
fn filter_schema_after_optimization() {
use datafusion_expr::cast;
let schema = Schema::new(vec![
Field::new("a", DataType::UInt64, false),
Field::new("b", DataType::UInt64, false),
Field::new("c", DataType::UInt64, false),
]);
let plan = table_scan(Some("table"), &schema, None)
.unwrap()
.filter(cast(col("a"), DataType::Int64).lt(lit(1_i64)).and(cast(col("a"), DataType::Int64).not_eq(lit(1_i64))))
.unwrap()
.build()
.unwrap();
let rule = CommonSubexprEliminate {};
let optimized_plan = rule.optimize(&plan, &mut OptimizerConfig::new()).unwrap();
/* The optimized_plan has an additional column for the cast; the datatype should be i64,
* but it's boolean instead.
*
* DFField {
* qualifier: None,
* field: Field {
* name: "CAST(#table.a AS Int64)#table.a",
* data_type: Boolean,
* nullable: true,
* dict_id: 0,
* dict_is_ordered: false,
* metadata: None,
* },
*},
*
*/
println!("{:#?}", optimized_plan);
println!("{:#?}", optimized_plan.schema());
panic!();
}In fn post_visit(mut self, expr: &Expr) -> Result<Self> {
self.series_number += 1;
let (idx, sub_expr_desc) = self.pop_enter_mark();
// skip exprs should not be recognize.
if matches!(
expr,
Expr::Literal(..)
| Expr::Column(..)
| Expr::ScalarVariable(..)
| Expr::Alias(..)
| Expr::Sort { .. }
| Expr::Wildcard
) {
self.id_array[idx].0 = self.series_number;
let desc = Self::desc_expr(expr);
self.visit_stack.push(VisitRecord::ExprItem(desc));
return Ok(self);
} let mut desc = Self::desc_expr(expr);
desc.push_str(&sub_expr_desc);
self.id_array[idx] = (self.series_number, desc.clone());
self.visit_stack.push(VisitRecord::ExprItem(desc.clone()));
// Error: data type of a sub-expression can be different from the final type.
// This leads to a wrong schema of the resulting logical plan.
let data_type = self.data_type.clone();
self.expr_set
.entry(desc)
.or_insert_with(|| (expr.clone(), 0, data_type))
.1 += 1;
Ok(self)
} |
|
@alex-natzka Could you please take a look the test I ignore this test, because it will produce unexpected result. cc @alamb @andygrove |
@liukun4515 looks like you needed to ignore the test for your previous PR #3636 . I'm a bit confused because, at least at first glance, it looks like the It may be safe to create a PR that un-ignores the test and updates the |
|
@liukun4515 , after staring at the actual output for a longer while, I realize that there is a double projection above the filter, which is indeed unexpected: The projection on the second line seems completely unnecessary. Sorry, seems like I solved a different problem than what you meant :/ Unfortunately, I'm not familiar enough with this optimization rule to fix this. At least it seems to me like the optimized logical plan is not wrong, just that the unnecessary projection is inefficient. |
|
Looks like I just tried and I get a more sensible execution plan in the test Changing the order of optimization rules here https://github.com/apache/arrow-datafusion/blob/61c38b7114e802f9f289bf5364a031395f5799a6/datafusion/optimizer/src/optimizer.rs#L138 like this yields the following test output: which looks better to me. |
I agree -- this is a very nice writeup @alex-natzka -- thank you (love the diagram). Shall we make a PR that proposes switching the order of the passes (and add comments explaining why we do filter pushdown first)? |
Describe the bug
after do this pr with moving the type coercion to the beginning for the optimizer, the
CommonSubexprEliminatewill generate the invalid projectionmaybe like #2907
I think many of the optimizer rule didn't take care of the
cast/try_castfor the type coercion.To Reproduce
Steps to reproduce the behavior:
Expected behavior
A clear and concise description of what you expected to happen.
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: