You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "vvivekiyer (via GitHub)" <gi...@apache.org> on 2023/02/21 03:33:40 UTC
[GitHub] [pinot] vvivekiyer opened a new pull request, #10314: [multistage] Fix return type for AVG aggregate function
vvivekiyer opened a new pull request, #10314:
URL: https://github.com/apache/pinot/pull/10314
Label = bugfix
Currently, for `AVG` aggregation function, Calcite determines the return type based on the input argument. This means that if the column if of INT type, the `AVG` return type would also be an integer.
The fix added in this PR returns a `DOUBLE` datatype for AVG.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10314:
URL: https://github.com/apache/pinot/pull/10314#issuecomment-1437835317
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10314?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@c0f6135`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10314 +/- ##
=========================================
Coverage ? 13.70%
Complexity ? 182
=========================================
Files ? 1961
Lines ? 107196
Branches ? 16393
=========================================
Hits ? 14687
Misses ? 91341
Partials ? 1168
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests2 | `13.70% <0.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10314?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...n/java/org/apache/pinot/query/type/TypeSystem.java](https://codecov.io/gh/apache/pinot/pull/10314?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdHlwZS9UeXBlU3lzdGVtLmphdmE=) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] somandal commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "somandal (via GitHub)" <gi...@apache.org>.
somandal commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1113319016
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
Review 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)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on PR #10314:
URL: https://github.com/apache/pinot/pull/10314#issuecomment-1439515773
Thanks for the fix
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1113880849
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
Review Comment:
Resolved all test issues.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1113735248
##########
pinot-query-runtime/src/test/resources/queries/Aggregates.json:
##########
@@ -20,11 +20,6 @@
}
},
"queries": [
- {
Review Comment:
Query on H2 database is returning an INT. Couldn't find a better workaround other than removing this test for H2.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1114451434
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
+ RelDataType argumentType) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
Review 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 AGG results are returning as double which is going to cause more issue than it resolves.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1113735248
##########
pinot-query-runtime/src/test/resources/queries/Aggregates.json:
##########
@@ -20,11 +20,6 @@
}
},
"queries": [
- {
Review 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.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] walterddr commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1114451434
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
+ RelDataType argumentType) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
Review 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.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
+ RelDataType argumentType) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
Review 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
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1114744775
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
+ RelDataType argumentType) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
Review Comment:
Based on offline discussion, created oss issue #10318 to determine the logic for AVG return type.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] vvivekiyer commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "vvivekiyer (via GitHub)" <gi...@apache.org>.
vvivekiyer commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1114691347
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/type/TypeSystem.java:
##########
@@ -50,4 +53,11 @@ public int getMaxNumericScale() {
public int getMaxNumericPrecision() {
return MAX_DECIMAL_PRECISION_DIGIT;
}
+
+ @Override
+ public RelDataType deriveAvgAggType(RelDataTypeFactory typeFactory,
+ RelDataType argumentType) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
Review 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
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a diff in pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia commented on code in PR #10314:
URL: https://github.com/apache/pinot/pull/10314#discussion_r1113894599
##########
pinot-query-runtime/src/test/resources/queries/Aggregates.json:
##########
@@ -20,11 +20,6 @@
}
},
"queries": [
- {
Review Comment:
Can we comment out this test for now instead of removing ?
Will `SELECT AVG(CAST int_col AS DOUBLE) FROM FOO` work ?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia merged pull request #10314: [multistage] Fix return type for AVG aggregate function
Posted by "siddharthteotia (via GitHub)" <gi...@apache.org>.
siddharthteotia merged PR #10314:
URL: https://github.com/apache/pinot/pull/10314
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org