support sum/avg agg for decimal, change sum(float32) --> float64

[liukun4515]

Which issue does this PR close?

part of #122

The result data type for decimal case (closes #1418)

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[liukun4515]

PTAL @alamb @houqp

[alamb]

Thank you for the contribution; Very nicely tested @liukun4515 🏅

I left some comments, but overall I think this is looking quite good

[alamb]

👍

[alamb]

Can you please document document the rationale for the 4 and 38 constants below (or even better pull them into named constants somewhere)?

I also don't understand where the additional 4 came from. I tried to see if it was what postgres did, but when I checked the output schema for avg(numeric(10,3)) appears to be numeric without the precision or scale specified 🤔

(arrow_dev) alamb@MacBook-Pro-2:~/Downloads$ psql
psql (14.1)
Type "help" for help.

alamb=# create table test(x decimal(10, 3));
CREATE TABLE
alamb=# insert into test values (1.02);
INSERT 0 1
alamb=# create table test2 as select avg(x) from test;
SELECT 1

alamb=# select table_name, column_name, numeric_precision, numeric_scale, data_type from information_schema.columns where table_name='test2';
 table_name | column_name | numeric_precision | numeric_scale | data_type 
------------+-------------+-------------------+---------------+-----------
 test2      | avg         |                   |               | numeric
(1 row)

[liukun4515]

This is the intention for #1418.
In the PG, we can create bigger precision for decimal than that in datafusion.
The decimal in datafusion, whose behavior may be different from that in PG.
So we should discuss them.
Now for the promotion precision or scale, I just follow the spark behavior.
@alamb

[alamb]

I think following the spark behavior is reasonable, but it should be documented (aka that the constant 4 came from spark). Otherwise in 3 months we'll be 🤔 why were the particular constants picked

[liukun4515]

add comments to the code and issue #1461 to track this rule.
We can add the rule to our document later in the follow-up pull request.

[alamb]

The comment above looks out of date -- I think it should simply be removed.

And perhaps we can change this code so it doesn't use unreachable as I think it would be fairly easy to reach this code by calling Avg::new(..) with some incorrect paramters

How about something like

assert!(matches!(data_type, DataType::Float64 | DataType::Decimal(_, _)));

Which I think might be easier to diagnose if anyone hits it

[liukun4515]

grate comments

[alamb]

if a sum of decimal(10,2) can be decimal(20,2) shouldn't the accumulator state also be decimal(20,2) to avoid overflow?

I think handling overflow is probably fine to for a later date / PR, but it is strange to me that there is a discrepancy between the type for sum and the accumulator type for computing avg

[liukun4515]

The result type of phy expr(sum/avg) is same with each Accumulator, and it was decided by sum_return_type and avg_return_type.

If the column is decimal(8,2), the avg of this column must be less than 10^8-1, but we need more digits to represent the decimal part. For example, The avg of 3,4,6 is 4.333333....., we should increase the scale part.

For the sum agg, we just should increase the precision part, and the rule of adding 10 to precision is spark coercion rule for sum decimal. We can have our rules for decimal if we want.
@alamb
We can just follow the spark now, and change the rules if we want to define own rules.

[liukun4515]

add issue to track the overflow.
#1460

[alamb]

👍

[alamb]

Suggested change

	(None, rhs) => {
	(None, Some(rhs)) => {

I think you could avoid the unwrap below (which does a redundant check) by using a pattern match here

[liukun4515]

Thanks, Done.

[alamb]

Suggested change

	(lhs, rhs) => {
	(Some(lhs), rhs) => {

Suggested change

	(lhs, rhs) => {
	(lhs, rhs) => {

[liukun4515]

Done

[alamb]

I think this comment also applies to sum, not just avg

[liukun4515]

I have changed the logic to handle diff scale values to sum, so the comments has been removed.

[alamb]

Suggested change

	// For avg aggravate function.
	// For avg aggregate function.

Suggested change

	// For avg aggravate function.
	// For avg aggravate function.

[liukun4515]

Done

[alamb]

I don't understand the need for this clause. It means, among other things, it would seem to make sum for decimal is not commutative which is confusing

I would expect that sum(lhs, rhs) == sum(rhs, lhs) for any specific lhs and rhs

[liukun4515]

This is also ugly and confusing to me.

I have refined this part.
For the input args, the left decimal value and right decimal value, there is two cases:

1. scale is same: just find the max precision as the result precision
2. scale is diff: select the max(scale1,scale2) as the result scale and max precision as the result precision.

[liukun4515]

let max_precision = p1.max(p2);
            if s1.eq(s2) {
                // s1 = s2
                sum_decimal(v1, v2, max_precision, s1)
            } else if s1.gt(s2) {
                // s1 > s2
                sum_decimal_with_diff_scale(v1, v2, max_precision, s1, s2)
            } else if s1.lt(s2) {
                // s1 < s2
                sum_decimal_with_diff_scale(v2, v1, max_precision, s2, s1)
            } else {
                return Err(DataFusionError::Internal(format!(
                    "Sum is not expected to receive lhs {:?}, rhs {:?}",
                    lhs, rhs
                )));
            }

[jimexist]

Suggested change

	// In the https://www.postgresql.org/docs/8.2/functions-aggregate.html doc,
	// In the https://www.postgresql.org/docs/current/functions-aggregate.html doc,

[liukun4515]

Done

[jimexist]

Suggested change

	(lhs, rhs) => {
	ScalarValue::Decimal128(Some(lhs.unwrap() + rhs.unwrap()), *precision, *scale)
	(Some(lhs), Some(rhs)) => {
	ScalarValue::Decimal128(Some(lhs + rhs), *precision, *scale)

[liukun4515]

Thanks, @jimexist updated from the @alamb comments.

[jimexist]

Suggested change

	match (lhs, rhs) {
	(None, None) => ScalarValue::Decimal128(None, *precision, *scale),
	(None, rhs) => ScalarValue::Decimal128(*rhs, *precision, *scale),
	(lhs, None) => ScalarValue::Decimal128(*lhs, *precision, *scale),
	(lhs, rhs) => {
	ScalarValue::Decimal128(Some(lhs.unwrap() + rhs.unwrap()), *precision, *scale)
	}
	}
	ScalarValue::Decimal128(Some(lhs.unwrap_or(0) + rhs.unwrap_or(0)), *precision, *scale)

[liukun4515]

Can this changes meet the None for left or right?

[liukun4515]

Change to

match (lhs, rhs) {
        (None, None) => ScalarValue::Decimal128(None, *precision, *scale),
        (None, rhs) => ScalarValue::Decimal128(*rhs, *precision, *scale),
        (lhs, None) => ScalarValue::Decimal128(*lhs, *precision, *scale),
        (Some(lhs_value), Some(rhs_value)) => {
            ScalarValue::Decimal128(Some(lhs_value + rhs_value), *precision, *scale)
        }
    }

[liukun4515]

In the comments, @alamb gives some comments about the precision or scale for the result data type.
I want to follow the guidance document https://www.postgresql.org/docs/8.2/functions-aggregate.html at first but can't find some useful suggestions.
So I follow the behavior in the spark sun and avg.

[alamb]

I want to follow the guidance document https://www.postgresql.org/docs/8.2/functions-aggregate.html at first but can't find some useful suggestions.
So I follow the behavior in the spark sun and avg.

As I mentioned above, i think following the behavior of spark is fine, but I think we should use symbolic constants (largely as a way to document why those particular constants were picked)

[liukun4515]

As I mentioned above, i think following the behavior of spark is fine, but I think we should use symbolic constants (largely as a way to document why those particular constants were picked)

Ok, I will refine this pull request.

[alamb]

👁️

[alamb]

👍

[alamb]

Moving to arrow-rs would be a good idea I think

[alamb]

I think it is looking good -- thank you @liukun4515 -- nice forward progress

[alamb]

Could potentially use make_decimal_type : https://github.com/apache/arrow-datafusion/blob/4b454d0363b034e3ebcdf88f6add2403cd77a23b/datafusion/src/sql/utils.rs#L510 here too