You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2025/01/16 22:11:50 UTC
[PR] Improve `case` expr constant handling [datafusion]
alamb opened a new pull request, #14159:
URL: https://github.com/apache/datafusion/pull/14159
Draft as it builds on https://github.com/apache/datafusion/pull/14156
## Which issue does this PR close?
- Follow on to https://github.com/apache/datafusion/pull/14156
## Rationale for this change
While reviewing https://github.com/apache/datafusion/pull/14156 I noticed that the code unecessairly expanded a constant to an array, and that I could update it avoid having any internal errors
## What changes are included in this PR?
1. New .slt tests
2. Update case handling code to make it clear the errors are unreachable
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
yes, with new slt tests
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve `case` expr constant handling for when `` [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on PR #14159:
URL: https://github.com/apache/datafusion/pull/14159#issuecomment-2599800359
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve `case` expr constant handling [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14159:
URL: https://github.com/apache/datafusion/pull/14159#discussion_r1921109048
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -345,34 +345,37 @@ impl CaseExpr {
let when_expr = &self.when_then_expr[0].0;
let then_expr = &self.when_then_expr[0].1;
- let when_expr_value = when_expr.evaluate(batch)?;
- let when_expr_value = match when_expr_value {
+ match when_expr.evaluate(batch)? {
+ // WHEN true --> column
+ ColumnarValue::Scalar(ScalarValue::Boolean(Some(true))) => {
+ then_expr.evaluate(batch)
+ }
+ // WHEN [false | null] --> NULL
ColumnarValue::Scalar(_) => {
- ColumnarValue::Array(when_expr_value.into_array(batch.num_rows())?)
Review Comment:
This line has the effect of creating an array out of a scalar -- I remove that and added the special case handling for the values
##########
datafusion/physical-expr/src/expressions/case.rs:
##########
@@ -345,34 +345,37 @@ impl CaseExpr {
let when_expr = &self.when_then_expr[0].0;
let then_expr = &self.when_then_expr[0].1;
- let when_expr_value = when_expr.evaluate(batch)?;
- let when_expr_value = match when_expr_value {
+ match when_expr.evaluate(batch)? {
+ // WHEN true --> column
+ ColumnarValue::Scalar(ScalarValue::Boolean(Some(true))) => {
+ then_expr.evaluate(batch)
+ }
+ // WHEN [false | null] --> NULL
ColumnarValue::Scalar(_) => {
- ColumnarValue::Array(when_expr_value.into_array(batch.num_rows())?)
+ // return scalar NULL value
+ ScalarValue::try_from(self.data_type(&batch.schema())?)
+ .map(ColumnarValue::Scalar)
}
- other => other,
- };
-
- if let ColumnarValue::Array(bit_mask) = when_expr_value {
- let bit_mask = bit_mask
- .as_any()
- .downcast_ref::<BooleanArray>()
- .expect("predicate should evaluate to a boolean array");
- // invert the bitmask
- let bit_mask = match bit_mask.null_count() {
- 0 => not(bit_mask)?,
- _ => not(&prep_null_mask_filter(bit_mask))?,
- };
- match then_expr.evaluate(batch)? {
- ColumnarValue::Array(array) => {
- Ok(ColumnarValue::Array(nullif(&array, &bit_mask)?))
- }
- ColumnarValue::Scalar(_) => {
- internal_err!("expression did not evaluate to an array")
+ // WHEN column --> column
+ ColumnarValue::Array(bit_mask) => {
+ let bit_mask = bit_mask
+ .as_any()
+ .downcast_ref::<BooleanArray>()
+ .expect("predicate should evaluate to a boolean array");
+ // invert the bitmask
+ let bit_mask = match bit_mask.null_count() {
+ 0 => not(bit_mask)?,
+ _ => not(&prep_null_mask_filter(bit_mask))?,
+ };
+ match then_expr.evaluate(batch)? {
+ ColumnarValue::Array(array) => {
+ Ok(ColumnarValue::Array(nullif(&array, &bit_mask)?))
+ }
+ ColumnarValue::Scalar(_) => {
+ internal_err!("expression did not evaluate to an array")
+ }
}
}
- } else {
Review Comment:
This is not actually possible as the previous version of the code expands out a constant to an array
```rust
ColumnarValue::Array(when_expr_value.into_array(batch.num_rows())?)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve `case` expr constant handling for when `` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #14159:
URL: https://github.com/apache/datafusion/pull/14159
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Improve `case` expr constant handling for when `` [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #14159:
URL: https://github.com/apache/datafusion/pull/14159#issuecomment-2601131708
Thanks @alamb @Omega359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org