[multistage] Fix return type for AVG aggregate function#10314
[multistage] Fix return type for AVG aggregate function#10314siddharthteotia merged 2 commits intoapache:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10314 +/- ##
=========================================
Coverage ? 70.27%
Complexity ? 5867
=========================================
Files ? 2027
Lines ? 110066
Branches ? 16716
=========================================
Hits ? 77349
Misses ? 27283
Partials ? 5434
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| } | ||
|
|
||
| @Override | ||
| public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory, |
There was a problem hiding this comment.
Great catch! You will need to make some changes to the WindowFunctionsPlans.json to pick up the casting in the plans (I've used AVG quite extensively in that file)
There was a problem hiding this comment.
Resolved all test issues.
vvivekiyer
force-pushed
the
multi-stage-avg
branch
from
bc7c1c6 to
9d8e3f9
Compare
| } | ||
| }, | ||
| "queries": [ | ||
| { |
There was a problem hiding this comment.
Query on H2 database is returning an INT. Couldn't find a better workaround other than removing this test for H2. Would appreciate inputs here.
There was a problem hiding this comment.
Can we comment out this test for now instead of removing ?
Will SELECT AVG(CAST int_col AS DOUBLE) FROM FOO work ?
|
Thanks for the fix |
| return typeFactory.createTypeWithNullability( | ||
| typeFactory.createSqlType(SqlTypeName.DOUBLE), false); |
There was a problem hiding this comment.
This is not the right way to fix the issue. we should rely on Calcite's derived type system to figure out what the return type should be. this way all AVG results are returning as double which is going to cause more issue than it resolves.
There was a problem hiding this comment.
based on a coarse read, we should check the input decimal precision and try to do upcast to the appropriate floating point type. (e.g. the scale and precision
There was a problem hiding this comment.
Thanks for the review! I resorted to using double datatype for AVG because that's what we do in V1 engine.
We could maybe do what postgres does. Thoughts?
numeric for any integer type argument, double precision for a floating-point argument, otherwise the same as the argument data type
There was a problem hiding this comment.
Based on offline discussion, created oss issue #10318 to determine the logic for AVG return type.
5 participants
Label = bugfix
Currently, for
AVGaggregation function, Calcite determines the return type based on the input argument. This means that if the column if of INT type, theAVGreturn type would also be an integer.The fix added in this PR returns a
DOUBLEdatatype for AVG.