-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix CASE evaluation for custom column expressions #24484
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,6 +133,7 @@ impl CaseBody { | |
| // Determine the set of columns that are used in all the expressions of the case body. | ||
| // Use an ordered set so lambda variables continue to be positioned after columns | ||
| let mut used_column_indices = BTreeSet::<usize>::new(); | ||
| let mut supports_projection = true; | ||
| let mut collect_column_indices = |expr: &Arc<dyn PhysicalExpr>| { | ||
| expr.apply(|expr| { | ||
| if let Some(column) = expr.downcast_ref::<Column>() { | ||
|
|
@@ -141,6 +142,11 @@ impl CaseBody { | |
| expr.downcast_ref::<LambdaVariable>() | ||
| { | ||
| used_column_indices.insert(lambda_variable.index()); | ||
| } else if expr.downcast_ref::<Literal>().is_none() | ||
| && expr.children().is_empty() | ||
| { | ||
| // Unknown leaves may read input columns without exposing a Column child. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks like a reasonable conservative fix for the reported crash. One process point though: the discussion in #21231 was also considering a This PR takes a narrower approach by treating unknown leaves as unsafe. I think that is fine as a targeted fix, but it would be helpful to say so explicitly in the PR description and use wording like |
||
| supports_projection = false; | ||
| } | ||
| Ok(TreeNodeRecursion::Continue) | ||
| }) | ||
|
|
@@ -216,6 +222,7 @@ impl CaseBody { | |
| Ok(ProjectedCaseBody { | ||
| projection, | ||
| body: projected_body, | ||
| supports_projection, | ||
| }) | ||
| } | ||
| } | ||
|
|
@@ -251,6 +258,7 @@ impl CaseBody { | |
| struct ProjectedCaseBody { | ||
| projection: Vec<usize>, | ||
| body: CaseBody, | ||
| supports_projection: bool, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we document the invariant on That relationship is important enough that a short field comment, plus a sentence in the |
||
| } | ||
|
|
||
| /// The CASE expression is similar to a series of nested if/else and there are two forms that | ||
|
|
@@ -1056,7 +1064,7 @@ impl CaseExpr { | |
| .copied() | ||
| .filter(|index| *index < batch.num_columns()) | ||
| .collect::<Vec<_>>(); | ||
| if projection.len() < batch.num_columns() { | ||
| if projected.supports_projection && projection.len() < batch.num_columns() { | ||
| let projected_batch = batch.project(&projection)?; | ||
| projected | ||
| .body | ||
|
|
@@ -1086,7 +1094,7 @@ impl CaseExpr { | |
| .copied() | ||
| .filter(|index| *index < batch.num_columns()) | ||
| .collect::<Vec<_>>(); | ||
| if projection.len() < batch.num_columns() { | ||
| if projected.supports_projection && projection.len() < batch.num_columns() { | ||
| let projected_batch = batch.project(&projection)?; | ||
| projected | ||
| .body | ||
|
|
@@ -1212,7 +1220,7 @@ impl CaseExpr { | |
| .copied() | ||
| .filter(|index| *index < batch.num_columns()) | ||
| .collect::<Vec<_>>(); | ||
| if projection.len() < batch.num_columns() { | ||
| if projected.supports_projection && projection.len() < batch.num_columns() { | ||
| // The case expressions do not use all the columns of the input batch. | ||
| // Project first to reduce time spent filtering. | ||
| let projected_batch = batch.project(&projection)?; | ||
|
|
@@ -1583,6 +1591,55 @@ mod tests { | |
| use datafusion_physical_expr_common::physical_expr::fmt_sql; | ||
| use half::f16; | ||
|
|
||
| #[derive(Debug, Hash, PartialEq, Eq)] | ||
| struct CustomColumn { | ||
| inner: Column, | ||
| } | ||
|
|
||
| impl CustomColumn { | ||
| fn new(name: &str, index: usize) -> Self { | ||
| Self { | ||
| inner: Column::new(name, index), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for CustomColumn { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| std::fmt::Display::fmt(&self.inner, f) | ||
| } | ||
| } | ||
|
|
||
| impl PhysicalExpr for CustomColumn { | ||
| fn data_type(&self, input_schema: &Schema) -> Result<DataType> { | ||
| self.inner.data_type(input_schema) | ||
| } | ||
|
|
||
| fn nullable(&self, input_schema: &Schema) -> Result<bool> { | ||
| self.inner.nullable(input_schema) | ||
| } | ||
|
|
||
| fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> { | ||
| self.inner.evaluate(batch) | ||
| } | ||
|
|
||
| fn children(&self) -> Vec<&Arc<dyn PhysicalExpr>> { | ||
| vec![] | ||
| } | ||
|
|
||
| fn with_new_children( | ||
| self: Arc<Self>, | ||
| children: Vec<Arc<dyn PhysicalExpr>>, | ||
| ) -> Result<Arc<dyn PhysicalExpr>> { | ||
| assert!(children.is_empty()); | ||
| Ok(self) | ||
| } | ||
|
|
||
| fn fmt_sql(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| self.inner.fmt_sql(f) | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn case_with_expr() -> Result<()> { | ||
| let batch = case_test_batch()?; | ||
|
|
@@ -1878,6 +1935,38 @@ mod tests { | |
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn case_without_expr_with_custom_column() -> Result<()> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice regression test. One thing I noticed is that this currently covers only the Could we add small tests for those two shapes too? A base-expression CASE using |
||
| let batch = case_test_batch()?; | ||
| let schema = batch.schema(); | ||
|
|
||
| let when1 = binary( | ||
| Arc::new(CustomColumn::new("a", 0)), | ||
| Operator::Eq, | ||
| lit("foo"), | ||
| &schema, | ||
| )?; | ||
| let when2 = binary( | ||
| Arc::new(CustomColumn::new("a", 0)), | ||
| Operator::Eq, | ||
| lit("bar"), | ||
| &schema, | ||
| )?; | ||
| let expr = generate_case_when_with_type_coercion( | ||
| None, | ||
| vec![(when1, lit(123i32)), (when2, lit(456i32))], | ||
| None, | ||
| schema.as_ref(), | ||
| )?; | ||
|
|
||
| let result = expr.evaluate(&batch)?.into_array(batch.num_rows())?; | ||
| let result = as_int32_array(&result)?; | ||
| let expected = Int32Array::from(vec![Some(123), None, None, Some(456)]); | ||
|
|
||
| assert_eq!(&expected, result); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn case_with_expr_when_null() -> Result<()> { | ||
| let batch = case_test_batch()?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this check is a little broader than the specific hazard we are trying to protect against. It will also disable CASE projection for some in-tree leaf expressions that cannot actually read the input batch positionally.
For example, a nullary
ScalarFunctionExprsuch asrandom()has no children, is not aLiteral, and is not const-folded because it is volatile. That means an expression likeCASE WHEN random() < 0.5 THEN a ELSE b ENDwould now lose the projection fast path on a wide batch.Would it make sense to keep known-safe DataFusion leaves projection-enabled, and reserve this fallback for expressions whose dependencies we genuinely cannot determine? If we prefer the conservative behavior for now, I think it would be worth calling out the performance tradeoff in the PR description.