You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/07 02:14:34 UTC
[GitHub] [pinot] agavra opened a new pull request, #9927: [DRAFT] check aggregate functions for type safety before running query
agavra opened a new pull request, #9927:
URL: https://github.com/apache/pinot/pull/9927
Fixes #8409 - if you issue a query with a function that does not match the expected signature you will get the following error message as response:
```
[
{
"errorCode": 200,
"message": "QueryExecutionError:\njava.lang.IllegalArgumentException: Function sum does not support input types: [STRING]\n\tat org.apache.pinot.core.query.utils.ExpressionTypeResolver.visitFunction(ExpressionTypeResolver.java:113)\n\tat org.apache.pinot.core.query.utils.ExpressionTypeResolver.visitFunction(ExpressionTypeResolver.java:47)\n\tat org.apache.pinot.common.request.context.ExpressionContext.visit(ExpressionContext.java:114)\n\tat org.apache.pinot.core.plan.maker.InstancePlanMakerImplV2.lambda$makeSegmentPlanNode$0(InstancePlanMakerImplV2.java:241)"
}
]
```
This PR works by introducing a visitor that traverses an Expression Context graph and resolves all input/output types recursively to make sure that aggregate functions can be properly resolved.
**NOTE:** this does not verify the validity of scalar or transform functions.
--
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 #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9927:
URL: https://github.com/apache/pinot/pull/9927#discussion_r1042576582
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -104,6 +104,19 @@ public void getColumns(Set<String> columns) {
}
}
+ public <T> T visit(ExpressionContextVisitor<T> visitor) {
Review Comment:
i felt the same.
--
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] agavra commented on a diff in pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9927:
URL: https://github.com/apache/pinot/pull/9927#discussion_r1042343609
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -104,6 +104,19 @@ public void getColumns(Set<String> columns) {
}
}
+ public <T> T visit(ExpressionContextVisitor<T> visitor) {
Review Comment:
+1 I agree - I think if people like this approach I will refactor the ExpressionContext to follow the classical pattern
--
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] gortiz commented on pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
gortiz commented on PR #9927:
URL: https://github.com/apache/pinot/pull/9927#issuecomment-1340540277
My two cents:
> "message": "QueryExecutionError:\njava.lang.IllegalArgumentException: Function sum does not support input types: [STRING] (...)
I think we should try to do not expose implementation dependent exceptions (like IllegalArgumentException) and instead just return the error message _Function sum does not support input types: [STRING]_
--
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] gortiz commented on pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
gortiz commented on PR #9927:
URL: https://github.com/apache/pinot/pull/9927#issuecomment-1340546388
I think this is a necessary feature and the code is clear.
--
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] gortiz commented on a diff in pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
gortiz commented on code in PR #9927:
URL: https://github.com/apache/pinot/pull/9927#discussion_r1041874334
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java:
##########
@@ -104,6 +104,19 @@ public void getColumns(Set<String> columns) {
}
}
+ public <T> T visit(ExpressionContextVisitor<T> visitor) {
Review Comment:
It is a little bit strange to me calling something a visitor without implementing the GoF visitor pattern, but I guess the problem here is the ExpressionContext class itself, which instead of being a sealed class with one subclass for each type, it is a single class with nullable attributes so... the classical GoF pattern cannot be applied as we use to.
--
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] agavra commented on a diff in pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
agavra commented on code in PR #9927:
URL: https://github.com/apache/pinot/pull/9927#discussion_r1041679689
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -235,6 +236,10 @@ private void applyQueryOptions(QueryContext queryContext) {
@Override
public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext queryContext) {
+ ExpressionTypeResolver validator =
+ new ExpressionTypeResolver(indexSegment, queryContext);
+ queryContext.getSelectExpressions().forEach(ec -> ec.visit(validator));
Review Comment:
Review Note: I didn't spend much time figuring out where to plug this in, this is definitely not an ideal place - just the easiest to prove that the approach works
--
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] agavra closed pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by "agavra (via GitHub)" <gi...@apache.org>.
agavra closed pull request #9927: [DRAFT] check aggregate functions for type safety before running query
URL: https://github.com/apache/pinot/pull/9927
--
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 pull request #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9927:
URL: https://github.com/apache/pinot/pull/9927#issuecomment-1343001464
CC @Jackie-Jiang to take a look
--
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 #9927: [DRAFT] check aggregate functions for type safety before running query
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9927:
URL: https://github.com/apache/pinot/pull/9927#issuecomment-1340303458
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9927?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9927](https://codecov.io/gh/apache/pinot/pull/9927?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fb06912) into [master](https://codecov.io/gh/apache/pinot/commit/e6a9881d7d1b397934983756ca4f14f3419d6824?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e6a9881) will **decrease** coverage by `53.16%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #9927 +/- ##
=============================================
- Coverage 68.97% 15.81% -53.17%
+ Complexity 5058 175 -4883
=============================================
Files 1982 1929 -53
Lines 106433 104137 -2296
Branches 16127 15864 -263
=============================================
- Hits 73409 16465 -56944
- Misses 27877 86469 +58592
+ Partials 5147 1203 -3944
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.81% <0.00%> (-0.02%)` | :arrow_down: |
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/9927?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/common/request/context/ExpressionContext.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vcmVxdWVzdC9jb250ZXh0L0V4cHJlc3Npb25Db250ZXh0LmphdmE=) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| [...a/org/apache/pinot/common/utils/PinotDataType.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvUGlub3REYXRhVHlwZS5qYXZh) | `0.00% <0.00%> (-81.50%)` | :arrow_down: |
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL21ha2VyL0luc3RhbmNlUGxhbk1ha2VySW1wbFYyLmphdmE=) | `0.00% <0.00%> (-79.42%)` | :arrow_down: |
| [...y/aggregation/function/AvgAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BdmdBZ2dyZWdhdGlvbkZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-81.68%)` | :arrow_down: |
| [...aggregation/function/AvgMVAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BdmdNVkFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ation/function/BaseBooleanAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9CYXNlQm9vbGVhbkFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-60.68%)` | :arrow_down: |
| [...aggregation/function/CountAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9Db3VudEFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-73.75%)` | :arrow_down: |
| [...gation/function/CovarianceAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9Db3ZhcmlhbmNlQWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-65.44%)` | :arrow_down: |
| [...regation/function/DistinctAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdEFnZ3JlZ2F0aW9uRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-47.06%)` | :arrow_down: |
| [...ion/function/DistinctCountAggregationFunction.java](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9EaXN0aW5jdENvdW50QWdncmVnYXRpb25GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-54.27%)` | :arrow_down: |
| ... and [1504 more](https://codecov.io/gh/apache/pinot/pull/9927/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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