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 2020/11/25 07:45:20 UTC
[GitHub] [incubator-pinot] siddharthteotia opened a new pull request #6288: Use sorted index based filtering only for dictionary encoded column
siddharthteotia opened a new pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288
Currently we build sorted index only if the column is dictionary encoded. However, when we write isSorted in on-disk segment metadata, we write on the basis of pre-index stats collector. So, for a sorted column without dictionary, segment metadata will indicate column as sorted
`properties.setProperty(getKeyFor(column, IS_SORTED), String.valueOf(columnIndexCreationInfo.isSorted()));`
During query processing, when we create filter operator, we check the data source metadata to see if the column is sorted and create sorted index based filter operator. However, using this operator for any sorted raw column will lead to the following error stack since we end up using a raw value based predicate evaluator for a dictionary based filter operator.
The solution is to do the additional check on data source to see if the column is dictionary encoded or not
`java.lang.UnsupportedOperationException
181762 at org.apache.pinot.core.operator.filter.predicate.BaseRawValueBasedPredicateEvaluator.getMatchingDictIds(BaseRawValueBasedPredicateEvaluator.java:40)
181763 at org.apache.pinot.core.operator.filter.SortedIndexBasedFilterOperator.getNextBlock(SortedIndexBasedFilterOperator.java:68)
181764 at org.apache.pinot.core.operator.filter.SortedIndexBasedFilterOperator.getNextBlock(SortedIndexBasedFilterOperator.java:35)
181765 at org.apache.pinot.core.operator.BaseOperator.nextBlock(BaseOperator.java:49)
181766 at org.apache.pinot.core.operator.DocIdSetOperator.getNextBlock(DocIdSetOperator.java:62)
181767 at org.apache.pinot.core.operator.DocIdSetOperator.getNextBlock(DocIdSetOperator.java:35)
181768 at org.apache.pinot.core.operator.BaseOperator.nextBlock(BaseOperator.java:49)
181769 at org.apache.pinot.core.operator.ProjectionOperator.getNextBlock(ProjectionOperator.java:57)
181770 at org.apache.pinot.core.operator.ProjectionOperator.getNextBlock(ProjectionOperator.java:30)`
----------------------------------------------------------------
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.
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-io edited a comment on pull request #6288: Use sorted index based filtering only for dictionary encoded column
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288#issuecomment-733897627
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=h1) Report
> Merging [#6288](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=desc) (a6535ca) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `7.56%`.
> The diff coverage is `68.27%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6288 +/- ##
==========================================
+ Coverage 66.44% 74.01% +7.56%
==========================================
Files 1075 1252 +177
Lines 54773 61203 +6430
Branches 8168 8864 +696
==========================================
+ Hits 36396 45300 +8904
+ Misses 15700 12988 -2712
- Partials 2677 2915 +238
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `45.82% <51.88%> (?)` | |
| unittests | `65.21% <45.12%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `44.44% <0.00%> (-4.40%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `78.57% <ø> (+5.40%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
| [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| [.../pinot/common/function/DateTimePatternHandler.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
| [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25EZWZpbml0aW9uUmVnaXN0cnkuamF2YQ==) | `88.88% <ø> (+44.44%)` | :arrow_up: |
| [...org/apache/pinot/common/function/FunctionInfo.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `100.00% <ø> (ø)` | |
| ... and [1033 more](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=footer). Last update [4a6e094...a6535ca](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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-io commented on pull request #6288: Use sorted index based filtering only for dictionary encoded column
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288#issuecomment-733897627
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=h1) Report
> Merging [#6288](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=desc) (a6535ca) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.22%`.
> The diff coverage is `45.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6288 +/- ##
==========================================
- Coverage 66.44% 65.21% -1.23%
==========================================
Files 1075 1252 +177
Lines 54773 61203 +6430
Branches 8168 8864 +696
==========================================
+ Hits 36396 39916 +3520
- Misses 15700 18447 +2747
- Partials 2677 2840 +163
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.21% <45.12%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [.../apache/pinot/common/exception/QueryException.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZXhjZXB0aW9uL1F1ZXJ5RXhjZXB0aW9uLmphdmE=) | `90.27% <ø> (+5.55%)` | :arrow_up: |
| [...pinot/common/function/AggregationFunctionType.java](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1098 more](https://codecov.io/gh/apache/incubator-pinot/pull/6288/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=footer). Last update [4a6e094...a6535ca](https://codecov.io/gh/apache/incubator-pinot/pull/6288?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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 #6288: Use sorted index based filtering only for dictionary encoded column
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288#discussion_r530590513
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java
##########
@@ -47,9 +47,15 @@ public static BaseFilterOperator getLeafFilterOperator(PredicateEvaluator predic
return new MatchAllFilterOperator(numDocs);
}
+ // Currently sorted index based filtering is supported only for
+ // dictionary encoded columns. The on-disk segment metadata
+ // will indicate if the column is sorted or not regardless of
+ // whether it is raw or dictionary encoded. Here when creating
+ // the filter operator, we need to make sure that sort filter
+ // operator is used only if the column is sorted and has dictionary.
Predicate.Type predicateType = predicateEvaluator.getPredicateType();
if (predicateType == Predicate.Type.RANGE) {
- if (dataSource.getDataSourceMetadata().isSorted()) {
+ if (dataSource.getDataSourceMetadata().isSorted() && (dataSource.getDictionary() != null)) {
Review comment:
(nit)
```suggestion
if (dataSource.getDataSourceMetadata().isSorted() && dataSource.getDictionary() != null) {
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java
##########
@@ -59,7 +65,7 @@ public static BaseFilterOperator getLeafFilterOperator(PredicateEvaluator predic
} else if (predicateType == Predicate.Type.REGEXP_LIKE) {
return new ScanBasedFilterOperator(predicateEvaluator, dataSource, numDocs);
} else {
- if (dataSource.getDataSourceMetadata().isSorted()) {
+ if (dataSource.getDataSourceMetadata().isSorted() && (dataSource.getDictionary() != null)) {
Review comment:
(nit)
```suggestion
if (dataSource.getDataSourceMetadata().isSorted() && dataSource.getDictionary() != null) {
```
----------------------------------------------------------------
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.
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] siddharthteotia commented on a change in pull request #6288: Use sorted index based filtering only for dictionary encoded column
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288#discussion_r532849542
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java
##########
@@ -47,9 +47,15 @@ public static BaseFilterOperator getLeafFilterOperator(PredicateEvaluator predic
return new MatchAllFilterOperator(numDocs);
}
+ // Currently sorted index based filtering is supported only for
+ // dictionary encoded columns. The on-disk segment metadata
+ // will indicate if the column is sorted or not regardless of
+ // whether it is raw or dictionary encoded. Here when creating
+ // the filter operator, we need to make sure that sort filter
+ // operator is used only if the column is sorted and has dictionary.
Predicate.Type predicateType = predicateEvaluator.getPredicateType();
if (predicateType == Predicate.Type.RANGE) {
- if (dataSource.getDataSourceMetadata().isSorted()) {
+ if (dataSource.getDataSourceMetadata().isSorted() && (dataSource.getDictionary() != null)) {
Review comment:
done
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/filter/FilterOperatorUtils.java
##########
@@ -59,7 +65,7 @@ public static BaseFilterOperator getLeafFilterOperator(PredicateEvaluator predic
} else if (predicateType == Predicate.Type.REGEXP_LIKE) {
return new ScanBasedFilterOperator(predicateEvaluator, dataSource, numDocs);
} else {
- if (dataSource.getDataSourceMetadata().isSorted()) {
+ if (dataSource.getDataSourceMetadata().isSorted() && (dataSource.getDictionary() != null)) {
Review comment:
done
----------------------------------------------------------------
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.
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] siddharthteotia merged pull request #6288: Use sorted index based filtering only for dictionary encoded column
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged pull request #6288:
URL: https://github.com/apache/incubator-pinot/pull/6288
----------------------------------------------------------------
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.
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