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 2021/07/05 19:47:49 UTC
[GitHub] [incubator-pinot] atris opened a new pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
atris opened a new pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129
## Description
<!-- Add a description of your PR here.
This PR introduces the notion of dictionary based execution for DISTINCT aggregates when the aggregate contains only a single, dictionary enabled column and no predicates.
-->
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options. YES
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
pinot.server.query.executor.use.dictionary.for.distinct -- Use this parameter to enable query planner to use dictionary based execution plan for single column, no predicate DISTINCT aggregates
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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] [incubator-pinot] codecov-commenter edited a comment on pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#issuecomment-874566291
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?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 [#7129](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d051a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/350307497aea000a30cfa130c3d66b6c8e824193?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3503074) will **increase** coverage by `0.02%`.
> The diff coverage is `55.00%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7129 +/- ##
============================================
+ Coverage 73.48% 73.50% +0.02%
- Complexity 91 92 +1
============================================
Files 1494 1497 +3
Lines 73576 73583 +7
Branches 10595 10632 +37
============================================
+ Hits 54069 54089 +20
+ Misses 15980 15954 -26
- Partials 3527 3540 +13
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.67% <8.12%> (+0.11%)` | :arrow_up: |
| unittests | `65.42% <55.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/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ombine/DistinctUsingDictionaryCombineOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Rpc3RpbmN0VXNpbmdEaWN0aW9uYXJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `26.92% <26.92%> (ø)` | |
| [.../pinot/core/query/reduce/ResultReducerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVzdWx0UmVkdWNlckZhY3RvcnkuamF2YQ==) | `81.81% <50.00%> (-7.08%)` | :arrow_down: |
| [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `87.50% <60.00%> (-2.34%)` | :arrow_down: |
| [...educe/DistinctUsingDictionaryDataTableReducer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3RVc2luZ0RpY3Rpb25hcnlEYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `63.63% <63.63%> (ø)` | |
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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=) | `86.48% <66.66%> (-2.52%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `93.79% <83.33%> (-0.50%)` | :arrow_down: |
| [...ator/query/DictionaryBasedAggregationOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9EaWN0aW9uYXJ5QmFzZWRBZ2dyZWdhdGlvbk9wZXJhdG9yLmphdmE=) | `88.73% <85.71%> (-0.93%)` | :arrow_down: |
| [.../core/plan/DictionaryBasedAggregationPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RpY3Rpb25hcnlCYXNlZEFnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `84.61% <100.00%> (+1.28%)` | :arrow_up: |
| [...aggregation/function/AggregationFunctionUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uVXRpbHMuamF2YQ==) | `88.67% <100.00%> (+0.21%)` | :arrow_up: |
| [...ot/segment/local/customobject/MinMaxRangePair.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvTWluTWF4UmFuZ2VQYWlyLmphdmE=) | `75.86% <0.00%> (-24.14%)` | :arrow_down: |
| ... and [116 more](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3503074...3d051a8](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] codecov-commenter edited a comment on pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#issuecomment-874566291
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?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 [#7129](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d051a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/350307497aea000a30cfa130c3d66b6c8e824193?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3503074) will **increase** coverage by `0.02%`.
> The diff coverage is `55.00%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7129 +/- ##
============================================
+ Coverage 73.48% 73.50% +0.02%
- Complexity 91 92 +1
============================================
Files 1494 1497 +3
Lines 73576 73583 +7
Branches 10595 10632 +37
============================================
+ Hits 54069 54089 +20
+ Misses 15980 15954 -26
- Partials 3527 3540 +13
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.67% <8.12%> (+0.11%)` | :arrow_up: |
| unittests | `65.42% <55.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/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ombine/DistinctUsingDictionaryCombineOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Rpc3RpbmN0VXNpbmdEaWN0aW9uYXJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `26.92% <26.92%> (ø)` | |
| [.../pinot/core/query/reduce/ResultReducerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVzdWx0UmVkdWNlckZhY3RvcnkuamF2YQ==) | `81.81% <50.00%> (-7.08%)` | :arrow_down: |
| [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `87.50% <60.00%> (-2.34%)` | :arrow_down: |
| [...educe/DistinctUsingDictionaryDataTableReducer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3RVc2luZ0RpY3Rpb25hcnlEYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `63.63% <63.63%> (ø)` | |
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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=) | `86.48% <66.66%> (-2.52%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `93.79% <83.33%> (-0.50%)` | :arrow_down: |
| [...ator/query/DictionaryBasedAggregationOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9EaWN0aW9uYXJ5QmFzZWRBZ2dyZWdhdGlvbk9wZXJhdG9yLmphdmE=) | `88.73% <85.71%> (-0.93%)` | :arrow_down: |
| [.../core/plan/DictionaryBasedAggregationPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RpY3Rpb25hcnlCYXNlZEFnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `84.61% <100.00%> (+1.28%)` | :arrow_up: |
| [...aggregation/function/AggregationFunctionUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uVXRpbHMuamF2YQ==) | `88.67% <100.00%> (+0.21%)` | :arrow_up: |
| [...ot/segment/local/customobject/MinMaxRangePair.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9jdXN0b21vYmplY3QvTWluTWF4UmFuZ2VQYWlyLmphdmE=) | `75.86% <0.00%> (-24.14%)` | :arrow_down: |
| ... and [116 more](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3503074...3d051a8](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] codecov-commenter commented on pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#issuecomment-874566291
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?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 [#7129](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d051a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/350307497aea000a30cfa130c3d66b6c8e824193?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3503074) will **decrease** coverage by `31.80%`.
> The diff coverage is `8.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7129 +/- ##
=============================================
- Coverage 73.48% 41.67% -31.81%
+ Complexity 91 7 -84
=============================================
Files 1494 1497 +3
Lines 73576 73583 +7
Branches 10595 10632 +37
=============================================
- Hits 54069 30669 -23400
- Misses 15980 40319 +24339
+ Partials 3527 2595 -932
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.67% <8.12%> (+0.11%)` | :arrow_up: |
| unittests | `?` | |
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/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ombine/DistinctUsingDictionaryCombineOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Rpc3RpbmN0VXNpbmdEaWN0aW9uYXJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...aggregation/function/AggregationFunctionUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uVXRpbHMuamF2YQ==) | `81.13% <0.00%> (-7.33%)` | :arrow_down: |
| [...educe/DistinctUsingDictionaryDataTableReducer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3RVc2luZ0RpY3Rpb25hcnlEYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../pinot/core/query/reduce/ResultReducerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVzdWx0UmVkdWNlckZhY3RvcnkuamF2YQ==) | `72.72% <0.00%> (-16.17%)` | :arrow_down: |
| [...ator/query/DictionaryBasedAggregationOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9EaWN0aW9uYXJ5QmFzZWRBZ2dyZWdhdGlvbk9wZXJhdG9yLmphdmE=) | `36.61% <14.28%> (-53.04%)` | :arrow_down: |
| [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `81.25% <20.00%> (-8.59%)` | :arrow_down: |
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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=) | `65.76% <25.00%> (-23.24%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `91.72% <83.33%> (-2.57%)` | :arrow_down: |
| [.../core/plan/DictionaryBasedAggregationPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RpY3Rpb25hcnlCYXNlZEFnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `84.61% <100.00%> (+1.28%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [984 more](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3503074...3d051a8](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] mayankshriv commented on a change in pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664755284
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -158,7 +170,8 @@ public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext quer
} else {
indexSegment.prefetch(queryContext.getColumns());
}
- if (QueryContextUtils.isAggregationQuery(queryContext)) {
+ if (QueryContextUtils.isAggregationQuery(queryContext) ||
Review comment:
Unclear on why the distinct query check needed here? There is also an `else` part.
--
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] [incubator-pinot] mayankshriv commented on a change in pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664751213
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -182,6 +195,11 @@ public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext quer
return new DictionaryBasedAggregationPlanNode(indexSegment, queryContext);
}
}
+
Review comment:
We should avoid special-casing 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] [incubator-pinot] Jackie-Jiang commented on a change in pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664935764
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java
##########
@@ -180,6 +180,7 @@ public static boolean isFitForDictionaryBasedComputation(String functionName) {
return functionName.equalsIgnoreCase(AggregationFunctionType.MIN.name())
|| functionName.equalsIgnoreCase(AggregationFunctionType.MAX.name())
|| functionName.equalsIgnoreCase(AggregationFunctionType.MINMAXRANGE.name())
+ || functionName.equalsIgnoreCase(AggregationFunctionType.DISTINCT.name())
Review comment:
`DISTINCT` is not modeled as aggregation on server side. We have it as a aggregation function type for historical reason, but we should not reach here
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -158,7 +170,8 @@ public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext quer
} else {
indexSegment.prefetch(queryContext.getColumns());
}
- if (QueryContextUtils.isAggregationQuery(queryContext)) {
+ if (QueryContextUtils.isAggregationQuery(queryContext) ||
+ (QueryContextUtils.isDistinctQuery(queryContext) && _useDictionaryForDistinct)) {
Review comment:
We have a branch for distinct queries (line 189 for existing code). You can check whether the query is fit for dictionary based plan similar to aggregation queries.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctUsingDictionaryCombineOperator.java
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.combine;
+
+import it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.util.AbstractCollection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.spi.utils.ByteArray;
+
+/**
+ * Operator which combines partial results for DISTINCT operation when being executed
+ * using the dictionary
+ */
+public class DistinctUsingDictionaryCombineOperator extends BaseCombineOperator {
Review comment:
+1. We should not need a separate combine operator
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -69,6 +69,10 @@
public static final String GROUPBY_TRIM_THRESHOLD = "groupby.trim.threshold";
public static final int DEFAULT_GROUPBY_TRIM_THRESHOLD = 1_000_000;
+ // set as pinot.server.query.executor.use.dictionary.for.distinct;
+ public static final String USE_DICTIONARY_FOR_DISTINCT = "use.dictionary.for.distinct";
Review comment:
This should not be configurable.
We should us the dictionary based operator when:
- There is no filter
- There is only one projection column
- The column is dictionary encoded
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -272,7 +274,20 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
}
QueryContext queryContext = BrokerRequestToQueryContextConverter.convert(brokerRequest);
- DataTableReducer dataTableReducer = ResultReducerFactory.getResultReducer(queryContext);
+
+ // Assuming all elements in the merged data table are of same type
+ Object internalObject = null;
+
+ if (QueryContextUtils.isDistinctQuery(queryContext)) {
Review comment:
There should be no change on broker side, server side should return the same response with/without the optimization. If not, it is protocol change and will cause backward incompatible issue
--
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] [incubator-pinot] codecov-commenter commented on pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#issuecomment-874566291
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?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 [#7129](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3d051a8) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/350307497aea000a30cfa130c3d66b6c8e824193?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3503074) will **decrease** coverage by `31.80%`.
> The diff coverage is `8.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #7129 +/- ##
=============================================
- Coverage 73.48% 41.67% -31.81%
+ Complexity 91 7 -84
=============================================
Files 1494 1497 +3
Lines 73576 73583 +7
Branches 10595 10632 +37
=============================================
- Hits 54069 30669 -23400
- Misses 15980 40319 +24339
+ Partials 3527 2595 -932
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `41.67% <8.12%> (+0.11%)` | :arrow_up: |
| unittests | `?` | |
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/incubator-pinot/pull/7129?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ombine/DistinctUsingDictionaryCombineOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0Rpc3RpbmN0VXNpbmdEaWN0aW9uYXJ5Q29tYmluZU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...aggregation/function/AggregationFunctionUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9mdW5jdGlvbi9BZ2dyZWdhdGlvbkZ1bmN0aW9uVXRpbHMuamF2YQ==) | `81.13% <0.00%> (-7.33%)` | :arrow_down: |
| [...educe/DistinctUsingDictionaryDataTableReducer.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvRGlzdGluY3RVc2luZ0RpY3Rpb25hcnlEYXRhVGFibGVSZWR1Y2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../pinot/core/query/reduce/ResultReducerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUmVzdWx0UmVkdWNlckZhY3RvcnkuamF2YQ==) | `72.72% <0.00%> (-16.17%)` | :arrow_down: |
| [...ator/query/DictionaryBasedAggregationOperator.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9EaWN0aW9uYXJ5QmFzZWRBZ2dyZWdhdGlvbk9wZXJhdG9yLmphdmE=) | `36.61% <14.28%> (-53.04%)` | :arrow_down: |
| [...va/org/apache/pinot/core/plan/CombinePlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0NvbWJpbmVQbGFuTm9kZS5qYXZh) | `81.25% <20.00%> (-8.59%)` | :arrow_down: |
| [...pinot/core/plan/maker/InstancePlanMakerImplV2.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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=) | `65.76% <25.00%> (-23.24%)` | :arrow_down: |
| [...e/pinot/core/query/reduce/BrokerReduceService.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvQnJva2VyUmVkdWNlU2VydmljZS5qYXZh) | `91.72% <83.33%> (-2.57%)` | :arrow_down: |
| [.../core/plan/DictionaryBasedAggregationPlanNode.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0RpY3Rpb25hcnlCYXNlZEFnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `84.61% <100.00%> (+1.28%)` | :arrow_up: |
| [...c/main/java/org/apache/pinot/common/tier/Tier.java](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [984 more](https://codecov.io/gh/apache/incubator-pinot/pull/7129/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3503074...3d051a8](https://codecov.io/gh/apache/incubator-pinot/pull/7129?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] [incubator-pinot] atris closed pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
atris closed pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129
--
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] [incubator-pinot] mayankshriv commented on a change in pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#discussion_r664737964
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -69,6 +69,10 @@
public static final String GROUPBY_TRIM_THRESHOLD = "groupby.trim.threshold";
public static final int DEFAULT_GROUPBY_TRIM_THRESHOLD = 1_000_000;
+ // set as pinot.server.query.executor.use.dictionary.for.distinct;
+ public static final String USE_DICTIONARY_FOR_DISTINCT = "use.dictionary.for.distinct";
+ public static final boolean DEFAULT_USE_DICTIONARY_FOR_DISTINCT = false;
Review comment:
What steps need to be done to make it ON by default?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/query/DictionaryBasedAggregationOperator.java
##########
@@ -80,46 +84,73 @@ protected IntermediateResultsBlock getNextBlock() {
aggregationResults
.add(new MinMaxRangePair(dictionary.getDoubleValue(0), dictionary.getDoubleValue(dictionarySize - 1)));
break;
+ case DISTINCT:
case DISTINCTCOUNT:
switch (dictionary.getValueType()) {
case INT:
IntOpenHashSet intSet = new IntOpenHashSet(dictionarySize);
for (int dictId = 0; dictId < dictionarySize; dictId++) {
+
+ // Enforce limits only for DISTINCT
+ if (aggregationFunction.getType() == DISTINCT && dictId == _limit) {
Review comment:
Would be (arguably) faster if we avoid the `if` inside of this for loop? That means we will have to have separate the `DISTINCT` and `DISTINCTCOUNT` cases.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -69,6 +69,10 @@
public static final String GROUPBY_TRIM_THRESHOLD = "groupby.trim.threshold";
public static final int DEFAULT_GROUPBY_TRIM_THRESHOLD = 1_000_000;
+ // set as pinot.server.query.executor.use.dictionary.for.distinct;
+ public static final String USE_DICTIONARY_FOR_DISTINCT = "use.dictionary.for.distinct";
Review comment:
One issue with server side configs is that different servers of the same table may potentially have different settings (due to user error). Perhaps this could be a query option, if we want to start with off-by-default?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctUsingDictionaryCombineOperator.java
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.combine;
+
+import it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.util.AbstractCollection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.spi.utils.ByteArray;
+
+/**
+ * Operator which combines partial results for DISTINCT operation when being executed
+ * using the dictionary
+ */
+public class DistinctUsingDictionaryCombineOperator extends BaseCombineOperator {
Review comment:
Shouldn't only the DistinctOperator be dictionary based at segment level (since dictionary id's are scoped within segment level)? If so, why do we need a combine and reduce operator for the dictionary based approach? Beyond segment level, we should be able to reuse the existing combine/reduce operators?
Nit: `DictionaryBasedCombineOperator` to match existing naming convention?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctUsingDictionaryCombineOperator.java
##########
@@ -0,0 +1,127 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.operator.combine;
+
+import it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.util.AbstractCollection;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import org.apache.pinot.core.common.Operator;
+import org.apache.pinot.core.operator.blocks.IntermediateResultsBlock;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.spi.utils.ByteArray;
+
+/**
+ * Operator which combines partial results for DISTINCT operation when being executed
+ * using the dictionary
+ */
+public class DistinctUsingDictionaryCombineOperator extends BaseCombineOperator {
+ private static final String OPERATOR_NAME = "DistinctUsingDictionaryCombineOperator";
+
+ private final boolean _hasOrderBy;
+
+ public DistinctUsingDictionaryCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
Review comment:
Some of these seem similar to `DistinctCombineOperator`, does it make sense fo this class to extend from it?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/DistinctUsingDictionaryDataTableReducer.java
##########
@@ -0,0 +1,164 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.reduce;
+
+import it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.io.Serializable;
+import java.util.AbstractCollection;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.common.response.broker.SelectionResults;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataTable;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.transport.ServerRoutingInstance;
+import org.apache.pinot.core.util.QueryOptions;
+
+/**
+ * Reducer for reducing partial results for DISTINCT operation when being executed using dictionary based plan
+ */
+public class DistinctUsingDictionaryDataTableReducer implements DataTableReducer {
Review comment:
Same nit for name `DictionaryBasedDistinctDataTableReducer`.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java
##########
@@ -272,7 +274,20 @@ public BrokerResponseNative reduceOnDataTable(BrokerRequest brokerRequest,
}
QueryContext queryContext = BrokerRequestToQueryContextConverter.convert(brokerRequest);
- DataTableReducer dataTableReducer = ResultReducerFactory.getResultReducer(queryContext);
+
+ // Assuming all elements in the merged data table are of same type
+ Object internalObject = null;
+
+ if (QueryContextUtils.isDistinctQuery(queryContext)) {
Review comment:
Hmm, any way to avoid this special casing here? I know we have special-cased DISTINCT in other places, but that is something we want to remove.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/DistinctUsingDictionaryDataTableReducer.java
##########
@@ -0,0 +1,164 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.core.query.reduce;
+
+import it.unimi.dsi.fastutil.doubles.DoubleOpenHashSet;
+import it.unimi.dsi.fastutil.floats.FloatOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.longs.LongOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import java.io.Serializable;
+import java.util.AbstractCollection;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import org.apache.pinot.common.metrics.BrokerMetrics;
+import org.apache.pinot.common.response.broker.BrokerResponseNative;
+import org.apache.pinot.common.response.broker.ResultTable;
+import org.apache.pinot.common.response.broker.SelectionResults;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataTable;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.request.context.QueryContext;
+import org.apache.pinot.core.transport.ServerRoutingInstance;
+import org.apache.pinot.core.util.QueryOptions;
+
+/**
+ * Reducer for reducing partial results for DISTINCT operation when being executed using dictionary based plan
+ */
+public class DistinctUsingDictionaryDataTableReducer implements DataTableReducer {
+ private final DistinctAggregationFunction _distinctAggregationFunction;
+ private final boolean _responseFormatSql;
+
+ // TODO: queryOptions.isPreserveType() is ignored for DISTINCT queries.
Review comment:
What's the implication of this? Is this a blocker for on-by-default?
--
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] [incubator-pinot] atris commented on pull request #7129: Support Dictionary Based Plan for DISTINCT Aggregates
Posted by GitBox <gi...@apache.org>.
atris commented on pull request #7129:
URL: https://github.com/apache/incubator-pinot/pull/7129#issuecomment-876233250
Superseded by https://github.com/apache/incubator-pinot/pull/7141
--
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