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

Add coercion rules for AggregateFunctions #1387

Merged

alamb merged 6 commits into apache:master from liukun4515:add_new_framework_expr_coercion

Dec 7, 2021

Contributor

liukun4515 commented Dec 1, 2021 •

edited

Loading

Which issue does this PR close?

part of #1356

We can extract two common function for all coercion rules, which are coerce_types and coerce_exprs.

The coerce_types is used to get the coerced result data type if the expr need to be added a tyr_cast.
The coerce_exprs is used to get the coerced exprs.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

liukun4515 added 2 commits

November 30, 2021 18:29


          upgrade the arrow-rs version

c01db7e


          new framework for type coercion

fc48c58

github-actions bot added ballista datafusion labels

liukun4515 changed the title ~~Add new framework expr coercion~~ refactor the expr coercion[aggregate expr function]

liukun4515 added 2 commits

December 1, 2021 10:15


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

7bc7eec

…_expr_coercion


          fix min/max accept the dict data type

42b2192

Contributor Author

liukun4515 commented Dec 1, 2021

@alamb PTAL
In the #1356, we reach the same idea to refactor the coercion framework and split the coercion rules to a single module.
This is the part for type coercion, and just for aggregate function.
If this approach is fine for you and the community, we can continue other refactoring work, such as scalar function expr, udf, UDAF.

alamb changed the title ~~refactor the expr coercion[aggregate expr function]~~ Add coercion rules for AggregateFunctions

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

Thanks @liukun4515 I really like where this is heading and it makes sense to me to have aggregate coercion rules that are separate from coercion rules for other functions.

I am curious how you see the overall code structure evolving. Is your idea that there will be several coercion modules, for each different Expr type, for example?

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs
datafusion/src/physical_plan/coercion_rule/function.rs
datafusion/src/physical_plan/coercion_rule/operators.rs

I had a few suggestions / comments, but overall I think this is looking almost ready to merge

Thank you for starting this work

datafusion/src/execution/context.rs Outdated

    
            @@ -2058,7 +2058,7 @@ mod tests {
          
                      .await

                      .unwrap_err();

                      assert_eq!(results.to_string(), "Error during planning: Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.");

                      assert_eq!(results.to_string(), "Error during planning: The function Sum do not support the Timestamp(Nanosecond, None).");

Contributor

alamb Dec 1, 2021

Would it be possible to add the valid signatures into this error message? The new wording is more readable, but we did lose some information about what type signatures are valid

Member

houqp Dec 2, 2021

I agree with this 👍

Contributor Author

liukun4515 Dec 7, 2021

In some SQL systems, if we input the incompatible datatype, they just throw the error or except and don't give the compatible data type.
We can refine this later if the supported data type is necessary for the user.

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs Outdated

+                  match signature.type_signature {
+                      TypeSignature::Uniform(agg_count, _) | TypeSignature::Any(agg_count) => {
+                          if input_types.len() != agg_count {
+                              return Err(DataFusionError::Plan(format!("The function {:?} expect argument number is {:?}, but the input argument number is {:?}",

Contributor

alamb Dec 1, 2021

Suggested change

      
                            return Err(DataFusionError::Plan(format!("The function {:?} expect argument number is {:?}, but the input argument number is {:?}",
          
                            return Err(DataFusionError::Plan(format!("The function {:?} expects {:?} arguments, but {:?} were provided",

Contributor Author

liukun4515 Dec 7, 2021

good suggestion.
Done

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs Outdated

+              use std::ops::Deref;
+              use std::sync::Arc;
+              pub fn coerce_types(

Contributor

alamb Dec 1, 2021

I think some docstrings would help here (perhaps a pointer to the module level documentation as is done in type_coercion.rs)

Contributor Author

liukun4515 Dec 7, 2021

Yes, I have added the doc for these two functions.

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs

+                          // TODO add checker, if the value type is complex data type
+                          Ok(vec![dict_value_type.deref().clone()])
+                      }
+                      // TODO add checker for datatype which min and max supported

Contributor

alamb Dec 1, 2021

that is a good TODO.

Contributor Author

liukun4515 Dec 7, 2021

Maybe we can add the checker in the follow-up pull request.
In this pull request, we just make consistent with before logic.

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs Show resolved Hide resolved

datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs

+                      let input_types = vec![
+                          vec![DataType::Int32],
+                          vec![DataType::Float32],
+                          // vec![DataType::Decimal(20, 3)],

Contributor

alamb Dec 1, 2021

likewise, here I think should work now

Contributor Author

liukun4515 Dec 7, 2021

I will support the decimal data type for SUM/AVG function in the feature #1408.
I have added the TODO to mark it.

datafusion/src/physical_plan/coercion_rule/mod.rs Outdated

+              // specific language governing permissions and limitations
+              // under the License.
+              //! define the coercion rule for different Expr type

Contributor

alamb Dec 1, 2021

Suggested change

      
            //! define the coercion rule for different Expr type
          
            //! define coercion rules for aggregate functions

datafusion/src/physical_plan/expressions/average.rs Outdated

                   }
               }
+              pub(crate) fn is_avg_support_arg_type(arg_type: &DataType) -> bool {
+                  // TODO support the interval
+                  // TODO: do we need to support the unsigned data type?

Contributor

alamb Dec 1, 2021

What do you mean by the unsigned data type? If you mean "do we support AVG(UInt8)" I think the answer is yes.

Contributor Author

liukun4515 Dec 7, 2021

got it.

datafusion/src/physical_plan/expressions/mod.rs

@@ @@ -60,6 +60,7 @@ pub mod helpers { @@
               pub use approx_distinct::ApproxDistinct;
               pub use array_agg::ArrayAgg;
+              pub(crate) use average::is_avg_support_arg_type;

Contributor

alamb Dec 1, 2021

I don't think we need to export this function (aka no need to declare it pub crate in this function). Likewise below

Contributor Author

liukun4515 Dec 7, 2021 •

edited

Loading

@alamb
But is_avg_support_arg_type will be used in other model, for example coercion_rule model.
https://github.com/apache/arrow-datafusion/blob/42b219215f1962bbf529c06392f5c7c978f70cc4/datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs#L75

datafusion/tests/sql.rs Outdated

-                  assert_eq!(err.to_string(), "Error during planning: Invalid or wrong number of arguments passed to aggregate: 'COUNT(DISTINCT )'");
+                  let logical_plan = ctx.create_logical_plan(sql);
+                  let err = logical_plan.unwrap_err();
+                  assert_eq!(err.to_string(), DataFusionError::Plan("The function Count expect argument number is 1, but the input argument number is 0".to_string()).to_string());

Contributor

alamb Dec 1, 2021

That is a nicer error message for sure 👍

Contributor Author

liukun4515 Dec 7, 2021

It will be changed to Error during planning: The function Count expects 1 arguments, but 0 were provided
After this comments https://github.com/apache/arrow-datafusion/pull/1387/files#r760575015

Contributor Author

liukun4515 commented Dec 2, 2021

@alamb I wish this refactor can involved many community developer or committer, and other developer can know this context.
I can remain this pull request for a while and don't merge it quickly.
What about your opinion?

Contributor Author

liukun4515 commented Dec 2, 2021

Thank you for your review and grammar suggestions.
I will address them later.

Contributor Author

liukun4515 commented Dec 2, 2021

but overall I think this is looking almost ready to merge

Thanks @liukun4515 I really like where this is heading and it makes sense to me to have aggregate coercion rules that are separate from coercion rules for other functions.

I am curious how you see the overall code structure evolving. Is your idea that there will be several coercion modules, for each different Expr type, for example?
datafusion/src/physical_plan/coercion_rule/aggregate_rule.rs
datafusion/src/physical_plan/coercion_rule/function.rs
datafusion/src/physical_plan/coercion_rule/operators.rs

Yes, this is the future result and diff type exprs have diff coercion rule in diff file.
The code structure will look like the example your provided.

I had a few suggestions / comments, but overall I think this is looking almost ready to merge

Thank you for starting this work

Contributor

alamb commented Dec 2, 2021

I can remain this pull request for a while and don't merge it quickly.

I think that is a good idea @liukun4515 -- though the danger of doing so is that we will end up with merge conflicts

Perhaps @Dandandan @houqp @rdettai or @jimexist would like to weigh in on the structure and direction as well

Member

xudong963 commented Dec 2, 2021

Thanks for your work! @liukun4515. If possible(the ticket doesn't be merged), I'll take a look at the weekend, because these days I am so busy with my job demands 🤦‍♂️, work 12 + hours per day ......

Contributor Author

liukun4515 commented Dec 3, 2021

@xudong963 I will begin to address the comments from next week.
This pull request is not urgent.

liukun4515 mentioned this pull request

The framework about expression type coercion #1356

Closed

7 tasks

xudong963 reviewed

View reviewed changes

datafusion/src/physical_plan/aggregates.rs Show resolved Hide resolved

Contributor

alamb commented Dec 6, 2021

@xudong963 and @liukun4515 what is the status of this PR? Shall we merge it? Is it waiting on more review / work?

Contributor Author

liukun4515 commented Dec 7, 2021

@xudong963 and @liukun4515 what is the status of this PR? Shall we merge it? Is it waiting on more review / work?

@alamb
I'm waiting for review comments these days.
But from today, I will address all comments.
After comments are addressed, I will ask someone to re-review.

liukun4515 mentioned this pull request

Implement DECIMAL type #122

Open


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

14d51da

…_expr_coercion

liukun4515 mentioned this pull request

Support decimal for min and max aggregate #1407

Merged


          address comments

17064f2

liukun4515 requested review from alamb and houqp

December 7, 2021 09:54

Contributor Author

liukun4515 commented Dec 7, 2021 •

edited

Loading

@alamb PTAL
It can be merged if it is looking good to you.

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

Thanks again @liukun4515 I think this is good to go.

It would be nice to clean up the tests but we can always do that as a follow on PR

datafusion/src/physical_plan/aggregates.rs

Comment on lines +420 to +442

+                                      let mut expect_type = data_type.clone();
+                                      if matches!(
+                                          data_type,
+                                          DataType::UInt8
+                                              | DataType::UInt16
+                                              | DataType::UInt32
+                                              | DataType::UInt64
+                                      ) {
+                                          expect_type = DataType::UInt64;
+                                      } else if matches!(
+                                          data_type,
+                                          DataType::Int8
+                                              | DataType::Int16
+                                              | DataType::Int32
+                                              | DataType::Int64
+                                      ) {
+                                          expect_type = DataType::Int64;
+                                      } else if matches!(
+                                          data_type,
+                                          DataType::Float32 | DataType::Float64
+                                      ) {
+                                          expect_type = data_type.clone();
+                                      }

Contributor

alamb Dec 7, 2021

Just FYI you can write this kind of logic in a more concise way with something like (untested and abbreviated)

let expect_type = match (data_type) {
  DataType::UInt8 | .... => DataType::UInt64,
  DataType::Int8 | .... => DataType::Int64,
  _ => data_type.clone()
}

Contributor

alamb Dec 7, 2021

#1416 <-- PR wth proposed cleanup

Contributor Author

liukun4515 Dec 8, 2021

good style and suggestion!

alamb merged commit 415c5e1 into apache:master

Contributor

alamb commented Dec 7, 2021

Thanks also to @xudong963 for the review

alamb mentioned this pull request

Minor test cleanup #1416

Merged

Member

houqp commented Dec 8, 2021

Thank you @liukun4515 for your contribution on this :)

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

Labels