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.

Sign up for GitHub

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

Jump to bottom

CaseWhen: coerce the all then and else data type to a common data type #2819

Merged

liukun4515 merged 5 commits into apache:master from liukun4515:fix_case_when_datatype_#2818

Jul 4, 2022

Contributor

liukun4515 commented Jun 30, 2022

Which issue does this PR close?

Closes #2818

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?


          case when: coerce to the same data type for the result data type

13d538c

liukun4515 marked this pull request as draft

June 30, 2022 14:02

github-actions bot added the physical-expr label

codecov-commenter commented Jun 30, 2022 •

edited

Loading

Codecov Report

Merging #2819 (de66710) into master (b47ab7c) will increase coverage by 0.01%.
The diff coverage is 98.78%.

@@            Coverage Diff             @@
##           master    #2819      +/-   ##
==========================================
+ Coverage   85.18%   85.19%   +0.01%     
==========================================
  Files         275      275              
  Lines       48564    48634      +70     
==========================================
+ Hits        41367    41434      +67     
- Misses       7197     7200       +3

Impacted Files	Coverage Δ
datafusion/physical-expr/src/expressions/case.rs	`92.75% <98.75%> (+0.75%)`	⬆️
datafusion/physical-expr/src/planner.rs	`92.36% <100.00%> (ø)`
datafusion/expr/src/logical_plan/plan.rs	`74.11% <0.00%> (-0.40%)`	⬇️
datafusion/core/src/physical_plan/metrics/value.rs	`87.43% <0.00%> (+0.50%)`	⬆️
datafusion/expr/src/window_frame.rs	`93.27% <0.00%> (+0.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b47ab7c...de66710. Read the comment docs.


          case when: support result type coerced

00edadf

liukun4515 force-pushed the fix_case_when_datatype_#2818 branch from 50fd76b to 00edadf Compare

July 1, 2022 07:31

liukun4515 marked this pull request as ready for review

July 1, 2022 07:31

liukun4515 requested review from alamb, yjshen and andygrove and removed request for alamb and yjshen

July 1, 2022 07:31


          Merge remote-tracking branch 'upstream/master' into fix_case_when_dat…

d50c671

…atype_#2818

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

Looks good to me -- I had some style suggestions but nothing major. Thank you @liukun4515

datafusion/physical-expr/src/expressions/case.rs

    
            @@ -76,7 +77,7 @@ impl CaseExpr {
          
                  /// Create a new CASE WHEN expression

                  pub fn try_new(

                      expr: Option<Arc<dyn PhysicalExpr>>,

                      when_then_expr: &[WhenThen],

                      when_then_expr: Vec<WhenThen>,

Contributor

alamb Jul 2, 2022

👍

datafusion/physical-expr/src/expressions/case.rs

                   Ok(Arc::new(CaseExpr::try_new(expr, when_thens, else_expr)?))
               }
+              fn get_case_common_type(
+                  when_thens: &[WhenThen],
+                  else_expr: Option<Arc<dyn PhysicalExpr>>,

Contributor

alamb Jul 2, 2022

Suggested change

      
                else_expr: Option<Arc<dyn PhysicalExpr>>,
          
                else_expr: Option<&PhysicalExpr>,

I wonder if there is a reason it needs to get a copy, or would a reference work too?

Contributor Author

liukun4515 Jul 3, 2022

The reference is not work for this.

datafusion/physical-expr/src/expressions/case.rs Show resolved Hide resolved

datafusion/physical-expr/src/expressions/case.rs Outdated

Comment on lines 309 to 315

+                          let left = when_thens
+                              .into_iter()
+                              .map(|(when, then)| {
+                                  let then = try_cast(then, input_schema, data_type.clone()).unwrap();
+                                  (when, then)
+                              })
+                              .collect::<Vec<WhenThen>>();

Contributor

alamb Jul 2, 2022

Are we sure this will always return without error (aka that this will not panic)?

Since this function can return an Result anyways, would it be better to return it. Something like (untested):

Suggested change

      
                        let left = when_thens
          
                            .into_iter()
          
                            .map(|(when, then)| {
          
                                let then = try_cast(then, input_schema, data_type.clone()).unwrap();
          
                                (when, then)
          
                            })
          
                            .collect::<Vec<WhenThen>>();
          
                       let left = when_thens
          
                           .into_iter()
          
                           .map(|(when, then)| {
          
                               let then = try_cast(then, input_schema, data_type.clone())?
          
                               Ok((when, then))
          
                           })
          
                           .collect::<Result<Vec<WhenThen>>>()?;

Contributor Author

liukun4515 Jul 3, 2022

good catch

datafusion/physical-expr/src/expressions/case.rs Outdated

+                      let when1 = binary(
+                          col("a", &schema)?,
+                          Operator::Eq,
+                          lit(ScalarValue::Utf8(Some("foo".to_string()))),

Contributor

alamb Jul 2, 2022

It would be nice if the the lit in the physical expr could use lit("foo") as well. But I tried it and it doesn't work

Suggested change

      
                        lit(ScalarValue::Utf8(Some("foo".to_string()))),
          
                        lit("foo"),

Contributor

alamb Jul 2, 2022

Contributor Author

liukun4515 Jul 3, 2022

I will rebase this pr after #2828 merged

Contributor Author

liukun4515 Jul 3, 2022

changed the code to use lit("foo") with merged #2828

alamb mentioned this pull request

Improve ergonomics of physical expr lit #2827

Closed

github-actions bot added the core label

alamb force-pushed the fix_case_when_datatype_#2818 branch from 00b0c39 to d50c671 Compare

July 2, 2022 10:35

Contributor

alamb commented Jul 2, 2022

I accidentally pushed commits to this branch -- I think I have removed them now. My apologies

liukun4515 added 2 commits

July 3, 2022 14:31


          Merge remote-tracking branch 'upstream/master' into fix_case_when_dat…

767f899

…atype_#2818


          change usage of literal

de66710

liukun4515 merged commit da392f4 into apache:master

This was referenced Jul 11, 2022

Error with CASE and DictionaryArrays: ArrowError(InvalidArgumentError("arguments need to have the same data type")) #2873

Closed

Fix casts of ScalarValue::Utf8 to DictionaryArray #2875

Closed

jonmmease mentioned this pull request

Update to DataFusion 10 vega/vegafusion#148

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core physical-expr