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/23 21:44:10 UTC
[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #6285: Improve performance for distinct queries
Jackie-Jiang opened a new pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285
## Description
Currently the distinct queries are handled as aggregation, which has limitations on early termination and special type handling for better performance.
This PR adds new operators for distinct queries.
The new operators:
- Early terminate the distinct only queries when enough distinct records are collected
- Specialize the handling for different data types, using primitives if possible
- Specialize the handling for single column distinct queries
- Inner segment, the operator will calculate the distinct records using dictionary ids if possible, and only read the dictionary when returning the results
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736826208
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=h1) Report
> Merging [#6285](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=desc) (e5440b1) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.09%`.
> The diff coverage is `45.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6285 +/- ##
==========================================
- Coverage 66.44% 65.35% -1.10%
==========================================
Files 1075 1281 +206
Lines 54773 61766 +6993
Branches 8168 8978 +810
==========================================
+ Hits 36396 40369 +3973
- Misses 15700 18516 +2816
- Partials 2677 2881 +204
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.35% <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/6285?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/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/6285/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/6285/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/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1128 more](https://codecov.io/gh/apache/incubator-pinot/pull/6285/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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/6285?src=pr&el=footer). Last update [0565f86...c42ec6c](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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 edited a comment on pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736826208
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=h1) Report
> Merging [#6285](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=desc) (c42ec6c) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.05%`.
> The diff coverage is `45.12%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6285 +/- ##
==========================================
- Coverage 66.44% 65.39% -1.06%
==========================================
Files 1075 1281 +206
Lines 54773 61768 +6995
Branches 8168 8977 +809
==========================================
+ Hits 36396 40392 +3996
- Misses 15700 18496 +2796
- Partials 2677 2880 +203
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.39% <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/6285?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/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/6285/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/6285/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/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vQWdncmVnYXRpb25GdW5jdGlvblR5cGUuamF2YQ==) | `98.27% <ø> (-1.73%)` | :arrow_down: |
| ... and [1128 more](https://codecov.io/gh/apache/incubator-pinot/pull/6285/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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/6285?src=pr&el=footer). Last update [0565f86...c42ec6c](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533710697
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/DistinctQueriesTest.java
##########
@@ -170,163 +199,360 @@ private ImmutableSegment createSegment(int index, List<GenericRow> records)
return ImmutableSegmentLoader.load(new File(INDEX_DIR, segmentName), ReadMode.mmap);
}
+ @Test
+ public void testSingleColumnDistinctInnerSegment() {
Review comment:
Split into `testSingleColumnDistinctOnlyInnerSegment` and `testSingleColumnDistinctOrderByInnerSegment`. I don't think we need to split further because that is not as easy to manage
----------------------------------------------------------------
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 merged pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533705136
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/distinct/DistinctExecutorFactory.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.distinct;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.core.operator.transform.TransformOperator;
+import org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedMultiColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedMultiColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawBytesSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawBytesSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawDoubleSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawDoubleSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawFloatSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawFloatSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawIntSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawIntSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawLongSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawLongSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawMultiColumnDistinctExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawStringSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawStringSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
+import org.apache.pinot.core.segment.index.readers.Dictionary;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+public class DistinctExecutorFactory {
+ private DistinctExecutorFactory() {
+ }
+
+ public static DistinctExecutor getDistinctExecutor(DistinctAggregationFunction distinctAggregationFunction,
+ TransformOperator transformOperator) {
+ List<ExpressionContext> expressions = distinctAggregationFunction.getInputExpressions();
+ List<OrderByExpressionContext> orderByExpressions = distinctAggregationFunction.getOrderByExpressions();
+ int limit = distinctAggregationFunction.getLimit();
+ if (orderByExpressions == 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 commented on a change in pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533680519
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/distinct/dictionary/BaseDictionaryBasedMultiColumnDistinctExecutor.java
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.distinct.dictionary;
+
+import it.unimi.dsi.fastutil.objects.ObjectOpenHashSet;
+import it.unimi.dsi.fastutil.objects.ObjectSet;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.query.distinct.DistinctTable;
+import org.apache.pinot.core.query.distinct.DistinctExecutor;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.segment.index.readers.Dictionary;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.ByteArray;
+
+
+abstract class BaseDictionaryBasedMultiColumnDistinctExecutor implements DistinctExecutor {
Review comment:
javadoc please
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/distinct/dictionary/BaseDictionaryBasedSingleColumnDistinctExecutor.java
##########
@@ -0,0 +1,69 @@
+/**
+ * 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.distinct.dictionary;
+
+import it.unimi.dsi.fastutil.ints.IntIterator;
+import it.unimi.dsi.fastutil.ints.IntOpenHashSet;
+import it.unimi.dsi.fastutil.ints.IntSet;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.query.distinct.DistinctTable;
+import org.apache.pinot.core.query.distinct.DistinctExecutor;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.segment.index.readers.Dictionary;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+import org.apache.pinot.spi.utils.ByteArray;
+
+
+abstract class BaseDictionaryBasedSingleColumnDistinctExecutor implements DistinctExecutor {
Review comment:
javadoc please
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533664885
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctCombineOperator.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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 java.util.Collections;
+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.distinct.DistinctTable;
+import org.apache.pinot.core.query.request.context.QueryContext;
+
+
+/**
+ * Combine operator for distinct queries.
+ */
+@SuppressWarnings("rawtypes")
+public class DistinctCombineOperator extends BaseCombineOperator {
+ private static final String OPERATOR_NAME = "DistinctCombineOperator";
+
+ public DistinctCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
+ }
+
+ @Override
+ public String getOperatorName() {
+ return OPERATOR_NAME;
+ }
+
+ @Override
+ protected boolean isQuerySatisfied(IntermediateResultsBlock resultsBlock) {
+ if (_queryContext.getOrderByExpressions() == null) {
+ List<Object> result = resultsBlock.getAggregationResult();
+ assert result != null && result.size() == 1 && result.get(0) instanceof DistinctTable;
+ DistinctTable distinctTable = (DistinctTable) result.get(0);
+ return distinctTable.size() >= _queryContext.getLimit();
+ } else {
+ return false;
+ }
+ }
+
+ @Override
+ protected void mergeResultsBlocks(IntermediateResultsBlock mergedBlock, IntermediateResultsBlock blockToMerge) {
+ // TODO: Use a separate way to represent DISTINCT instead of aggregation.
+ List<Object> mergedResults = mergedBlock.getAggregationResult();
+ assert mergedResults != null && mergedResults.size() == 1 && mergedResults.get(0) instanceof DistinctTable;
Review comment:
Why not use Preconditions.checkState()? Are we moving away from them as a convention?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctCombineOperator.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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 java.util.Collections;
+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.distinct.DistinctTable;
+import org.apache.pinot.core.query.request.context.QueryContext;
+
+
+/**
+ * Combine operator for distinct queries.
+ */
+@SuppressWarnings("rawtypes")
+public class DistinctCombineOperator extends BaseCombineOperator {
+ private static final String OPERATOR_NAME = "DistinctCombineOperator";
+
+ public DistinctCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
+ }
+
+ @Override
+ public String getOperatorName() {
+ return OPERATOR_NAME;
+ }
+
+ @Override
+ protected boolean isQuerySatisfied(IntermediateResultsBlock resultsBlock) {
+ if (_queryContext.getOrderByExpressions() == null) {
+ List<Object> result = resultsBlock.getAggregationResult();
Review comment:
You can probably stash away the order by presence in a boolean instance variable and avoid the method call to `getOrderByExpressions()` here since I believe it will be called by each combine thread for each segment it is processing in a loop. Simply check for the instance variable
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533656258
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctAggregationFunction.java
##########
@@ -18,31 +18,25 @@
*/
package org.apache.pinot.core.query.aggregation.function;
-import com.google.common.base.Preconditions;
-import java.util.Arrays;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.pinot.common.function.AggregationFunctionType;
-import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.common.BlockValSet;
-import org.apache.pinot.core.common.RowBasedBlockValueFetcher;
-import org.apache.pinot.core.data.table.Record;
import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.function.customobject.DistinctTable;
import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
import org.apache.pinot.core.query.request.context.ExpressionContext;
import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
/**
- * The DISTINCT clause in SQL is executed as the DISTINCT aggregation function.
- * TODO: Support group-by
+ * The DISTINCT clause in SQL is represented as the DISTINCT aggregation function. Currently it is only used to wrap the
+ * information for the distinct queries.
+ * TODO: Use a separate way to represent DISTINCT instead of aggregation.
*/
@SuppressWarnings("rawtypes")
-public class DistinctAggregationFunction implements AggregationFunction<DistinctTable, Comparable> {
+public class DistinctAggregationFunction implements AggregationFunction<Object, Comparable> {
Review comment:
Yes, currently `DISTINCT` keyword is not supported in the `PinotQuery`, so we use aggregation to pass the arguments. We keep it the same way for backward-compatibility, and I have added a TODO to clean it up later.
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533667722
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/distinct/DistinctExecutorFactory.java
##########
@@ -0,0 +1,176 @@
+/**
+ * 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.distinct;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.pinot.core.operator.transform.TransformOperator;
+import org.apache.pinot.core.query.aggregation.function.DistinctAggregationFunction;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedMultiColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedMultiColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.dictionary.DictionaryBasedSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawBytesSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawBytesSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawDoubleSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawDoubleSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawFloatSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawFloatSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawIntSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawIntSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawLongSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawLongSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawMultiColumnDistinctExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawStringSingleColumnDistinctOnlyExecutor;
+import org.apache.pinot.core.query.distinct.raw.RawStringSingleColumnDistinctOrderByExecutor;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
+import org.apache.pinot.core.segment.index.readers.Dictionary;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+public class DistinctExecutorFactory {
+ private DistinctExecutorFactory() {
+ }
+
+ public static DistinctExecutor getDistinctExecutor(DistinctAggregationFunction distinctAggregationFunction,
+ TransformOperator transformOperator) {
+ List<ExpressionContext> expressions = distinctAggregationFunction.getInputExpressions();
+ List<OrderByExpressionContext> orderByExpressions = distinctAggregationFunction.getOrderByExpressions();
+ int limit = distinctAggregationFunction.getLimit();
+ if (orderByExpressions == null) {
Review comment:
please consider separating this into multiple private methods for better readability
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533696596
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctCombineOperator.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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 java.util.Collections;
+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.distinct.DistinctTable;
+import org.apache.pinot.core.query.request.context.QueryContext;
+
+
+/**
+ * Combine operator for distinct queries.
+ */
+@SuppressWarnings("rawtypes")
+public class DistinctCombineOperator extends BaseCombineOperator {
+ private static final String OPERATOR_NAME = "DistinctCombineOperator";
+
+ public DistinctCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
+ }
+
+ @Override
+ public String getOperatorName() {
+ return OPERATOR_NAME;
+ }
+
+ @Override
+ protected boolean isQuerySatisfied(IntermediateResultsBlock resultsBlock) {
+ if (_queryContext.getOrderByExpressions() == null) {
+ List<Object> result = resultsBlock.getAggregationResult();
+ assert result != null && result.size() == 1 && result.get(0) instanceof DistinctTable;
+ DistinctTable distinctTable = (DistinctTable) result.get(0);
+ return distinctTable.size() >= _queryContext.getLimit();
+ } else {
+ return false;
+ }
+ }
+
+ @Override
+ protected void mergeResultsBlocks(IntermediateResultsBlock mergedBlock, IntermediateResultsBlock blockToMerge) {
+ // TODO: Use a separate way to represent DISTINCT instead of aggregation.
+ List<Object> mergedResults = mergedBlock.getAggregationResult();
+ assert mergedResults != null && mergedResults.size() == 1 && mergedResults.get(0) instanceof DistinctTable;
Review comment:
assert has no performance impact on production code. We don't really need to check it, this is more for readability
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533654379
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java
##########
@@ -172,7 +173,19 @@ public CombinePlanNode(List<PlanNode> planNodes, QueryContext queryContext, Exec
return new StreamingSelectionOnlyCombineOperator(operators, _queryContext, _executorService, _endTimeMs,
_streamObserver);
}
- if (QueryContextUtils.isAggregationQuery(_queryContext)) {
+ if (QueryContextUtils.isSelectionQuery(_queryContext)) {
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 commented on a change in pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533659152
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctAggregationFunction.java
##########
@@ -18,31 +18,25 @@
*/
package org.apache.pinot.core.query.aggregation.function;
-import com.google.common.base.Preconditions;
-import java.util.Arrays;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.pinot.common.function.AggregationFunctionType;
-import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.common.BlockValSet;
-import org.apache.pinot.core.common.RowBasedBlockValueFetcher;
-import org.apache.pinot.core.data.table.Record;
import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.function.customobject.DistinctTable;
import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
import org.apache.pinot.core.query.request.context.ExpressionContext;
import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
/**
- * The DISTINCT clause in SQL is executed as the DISTINCT aggregation function.
- * TODO: Support group-by
+ * The DISTINCT clause in SQL is represented as the DISTINCT aggregation function. Currently it is only used to wrap the
+ * information for the distinct queries.
+ * TODO: Use a separate way to represent DISTINCT instead of aggregation.
*/
@SuppressWarnings("rawtypes")
-public class DistinctAggregationFunction implements AggregationFunction<DistinctTable, Comparable> {
+public class DistinctAggregationFunction implements AggregationFunction<Object, Comparable> {
Review comment:
DISTINCT is supported in PinotQuery as a function. The CalciteSqlParser compiles Distinct specifically as a function expression.
```
private static List<Expression> convertDistinctSelectList(SqlNodeList selectList) {
List<Expression> selectExpr = new ArrayList<>();
selectExpr.add(convertDistinctAndSelectListToFunctionExpression(selectList));
return selectExpr;
}
```
----------------------------------------------------------------
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] mayankshriv edited a comment on pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
mayankshriv edited a comment on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736679804
Some of the changes (special-casing) seem motivated by performance improvement. If so, are there any performance improvement numbers to share? Also, are there any cases where we foresee the new implementation to be slower than the existing one?
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533706326
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InnerSegmentAggregationSingleValueQueriesTest.java
##########
@@ -175,26 +178,23 @@ public void testVeryLargeAggregationGroupBy() {
@Test
public void testSingleColumnDistinct() {
Review comment:
`DistinctQueriesTest` has all the scenarios covered
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533681779
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/distinct/raw/RawMultiColumnDistinctExecutor.java
##########
@@ -0,0 +1,83 @@
+/**
+ * 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.distinct.raw;
+
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.common.utils.DataSchema;
+import org.apache.pinot.core.common.BlockValSet;
+import org.apache.pinot.core.common.RowBasedBlockValueFetcher;
+import org.apache.pinot.core.data.table.Record;
+import org.apache.pinot.core.operator.blocks.TransformBlock;
+import org.apache.pinot.core.query.distinct.DistinctTable;
+import org.apache.pinot.core.query.distinct.DistinctExecutor;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
+import org.apache.pinot.spi.data.FieldSpec.DataType;
+
+
+public class RawMultiColumnDistinctExecutor implements DistinctExecutor {
Review comment:
Please add javadocs to all classes
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533683065
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/InnerSegmentAggregationSingleValueQueriesTest.java
##########
@@ -175,26 +178,23 @@ public void testVeryLargeAggregationGroupBy() {
@Test
public void testSingleColumnDistinct() {
Review comment:
Can we add another 2 similar tests for single and multi column distinct on dictionary encoded columns. Here both the tests are on raw columns (and so we use DistinctTable)
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736826208
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=h1) Report
> Merging [#6285](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=desc) (c42ec6c) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **increase** coverage by `7.72%`.
> The diff coverage is `68.27%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6285?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6285 +/- ##
==========================================
+ Coverage 66.44% 74.17% +7.72%
==========================================
Files 1075 1281 +206
Lines 54773 61768 +6995
Branches 8168 8977 +809
==========================================
+ Hits 36396 45817 +9421
+ Misses 15700 12990 -2710
- Partials 2677 2961 +284
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `45.68% <51.88%> (?)` | |
| unittests | `65.39% <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/6285?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/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/6285/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/6285/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/6285/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/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRGF0ZVRpbWVQYXR0ZXJuSGFuZGxlci5qYXZh) | `83.33% <ø> (ø)` | |
| [...ot/common/function/FunctionDefinitionRegistry.java](https://codecov.io/gh/apache/incubator-pinot/pull/6285/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/6285/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vZnVuY3Rpb24vRnVuY3Rpb25JbmZvLmphdmE=) | `100.00% <ø> (ø)` | |
| ... and [1068 more](https://codecov.io/gh/apache/incubator-pinot/pull/6285/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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/6285?src=pr&el=footer). Last update [0565f86...c42ec6c](https://codecov.io/gh/apache/incubator-pinot/pull/6285?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] mayankshriv commented on a change in pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533563925
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctAggregationFunction.java
##########
@@ -18,31 +18,25 @@
*/
package org.apache.pinot.core.query.aggregation.function;
-import com.google.common.base.Preconditions;
-import java.util.Arrays;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
import org.apache.pinot.common.function.AggregationFunctionType;
-import org.apache.pinot.common.utils.DataSchema;
import org.apache.pinot.common.utils.DataSchema.ColumnDataType;
import org.apache.pinot.core.common.BlockValSet;
-import org.apache.pinot.core.common.RowBasedBlockValueFetcher;
-import org.apache.pinot.core.data.table.Record;
import org.apache.pinot.core.query.aggregation.AggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.ObjectAggregationResultHolder;
-import org.apache.pinot.core.query.aggregation.function.customobject.DistinctTable;
import org.apache.pinot.core.query.aggregation.groupby.GroupByResultHolder;
import org.apache.pinot.core.query.request.context.ExpressionContext;
import org.apache.pinot.core.query.request.context.OrderByExpressionContext;
/**
- * The DISTINCT clause in SQL is executed as the DISTINCT aggregation function.
- * TODO: Support group-by
+ * The DISTINCT clause in SQL is represented as the DISTINCT aggregation function. Currently it is only used to wrap the
+ * information for the distinct queries.
+ * TODO: Use a separate way to represent DISTINCT instead of aggregation.
*/
@SuppressWarnings("rawtypes")
-public class DistinctAggregationFunction implements AggregationFunction<DistinctTable, Comparable> {
+public class DistinctAggregationFunction implements AggregationFunction<Object, Comparable> {
Review comment:
Seems this class does not implement any of the methods?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -123,8 +124,13 @@ public Plan makeInstancePlan(List<IndexSegment> indexSegments, QueryContext quer
@Override
public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext queryContext) {
- if (QueryContextUtils.isAggregationQuery(queryContext)) {
- // Aggregation query
+ if (QueryContextUtils.isSelectionQuery(queryContext)) {
Review comment:
Same here, common case first?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/CombinePlanNode.java
##########
@@ -172,7 +173,19 @@ public CombinePlanNode(List<PlanNode> planNodes, QueryContext queryContext, Exec
return new StreamingSelectionOnlyCombineOperator(operators, _queryContext, _executorService, _endTimeMs,
_streamObserver);
}
- if (QueryContextUtils.isAggregationQuery(_queryContext)) {
+ if (QueryContextUtils.isSelectionQuery(_queryContext)) {
Review comment:
Common case (of aggr/group-by) should be first?
----------------------------------------------------------------
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 pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736765873
@mayankshriv Thanks for the review. Addressed all the comments and updated the description to include the number for a simple distinct query. All the cases should perform at least the same as before (when there are multiple distinct columns and has raw index column, we fall back to the old way of using `DistinctTable` to merge raw records)
----------------------------------------------------------------
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] mayankshriv commented on pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
mayankshriv commented on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736679804
Some of the changes (special-casing) seem motivated by performance improvement. If so, are there any performance improvement numbers to share?
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533699018
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/DistinctCombineOperator.java
##########
@@ -0,0 +1,82 @@
+/**
+ * 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 java.util.Collections;
+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.distinct.DistinctTable;
+import org.apache.pinot.core.query.request.context.QueryContext;
+
+
+/**
+ * Combine operator for distinct queries.
+ */
+@SuppressWarnings("rawtypes")
+public class DistinctCombineOperator extends BaseCombineOperator {
+ private static final String OPERATOR_NAME = "DistinctCombineOperator";
+
+ public DistinctCombineOperator(List<Operator> operators, QueryContext queryContext, ExecutorService executorService,
+ long endTimeMs) {
+ super(operators, queryContext, executorService, endTimeMs);
+ }
+
+ @Override
+ public String getOperatorName() {
+ return OPERATOR_NAME;
+ }
+
+ @Override
+ protected boolean isQuerySatisfied(IntermediateResultsBlock resultsBlock) {
+ if (_queryContext.getOrderByExpressions() == null) {
+ List<Object> result = resultsBlock.getAggregationResult();
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 commented on a change in pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533688340
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/DistinctQueriesTest.java
##########
@@ -170,163 +199,360 @@ private ImmutableSegment createSegment(int index, List<GenericRow> records)
return ImmutableSegmentLoader.load(new File(INDEX_DIR, segmentName), ReadMode.mmap);
}
+ @Test
+ public void testSingleColumnDistinctInnerSegment() {
Review comment:
We should separate this into multiple individual unit tests.
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533654454
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java
##########
@@ -123,8 +124,13 @@ public Plan makeInstancePlan(List<IndexSegment> indexSegments, QueryContext quer
@Override
public PlanNode makeSegmentPlanNode(IndexSegment indexSegment, QueryContext queryContext) {
- if (QueryContextUtils.isAggregationQuery(queryContext)) {
- // Aggregation query
+ if (QueryContextUtils.isSelectionQuery(queryContext)) {
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] Jackie-Jiang commented on pull request #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#issuecomment-736818642
@siddharthteotia Addressed all the comments, please take another 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.
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533711499
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/DistinctQueriesTest.java
##########
@@ -394,285 +614,273 @@ private DistinctTable getDistinctTableInnerSegment(String query, boolean isPql)
* </li>
* </ul>
*/
- private void testDistinctInterSegmentHelper(String[] pqlQueries, String[] sqlQueries)
- throws Exception {
- ImmutableSegment segment0 = createSegment(0, generateRecords(0));
- ImmutableSegment segment1 = createSegment(1, generateRecords(1000));
- _indexSegments = Arrays.asList(segment0, segment1);
- try {
- {
- // Test selecting all columns
- String pqlQuery = pqlQueries[0];
- String sqlQuery = sqlQueries[0];
-
- // Check data schema
- BrokerResponseNative pqlResponse = getBrokerResponseForPqlQuery(pqlQuery);
- SelectionResults selectionResults = pqlResponse.getSelectionResults();
- assertNotNull(selectionResults);
- assertEquals(selectionResults.getColumns(),
- Arrays.asList("intColumn", "longColumn", "floatColumn", "doubleColumn", "stringColumn", "bytesColumn"));
- BrokerResponseNative sqlResponse = getBrokerResponseForSqlQuery(sqlQuery);
- ResultTable resultTable = sqlResponse.getResultTable();
- assertNotNull(resultTable);
- DataSchema dataSchema = resultTable.getDataSchema();
- assertEquals(dataSchema.getColumnNames(),
- new String[]{"intColumn", "longColumn", "floatColumn", "doubleColumn", "stringColumn", "bytesColumn"});
- assertEquals(dataSchema.getColumnDataTypes(),
- new ColumnDataType[]{ColumnDataType.INT, ColumnDataType.LONG, ColumnDataType.FLOAT, ColumnDataType.DOUBLE, ColumnDataType.STRING, ColumnDataType.BYTES});
-
- // Check values, where all 200 unique values should be returned
- List<Serializable[]> pqlRows = selectionResults.getRows();
- assertEquals(pqlRows.size(), 2 * NUM_UNIQUE_RECORDS_PER_SEGMENT);
- List<Object[]> sqlRows = resultTable.getRows();
- assertEquals(sqlRows.size(), 2 * NUM_UNIQUE_RECORDS_PER_SEGMENT);
- Set<Integer> expectedValues = new HashSet<>();
- for (int i = 0; i < NUM_UNIQUE_RECORDS_PER_SEGMENT; i++) {
+ private void testDistinctInterSegmentHelper(String[] pqlQueries, String[] sqlQueries) {
+ {
+ // Test selecting all columns
+ String pqlQuery = pqlQueries[0];
+ String sqlQuery = sqlQueries[0];
+
+ // Check data schema
+ BrokerResponseNative pqlResponse = getBrokerResponseForPqlQuery(pqlQuery);
Review comment:
Why do we need that? On the server side, PQL and SQL are handled the same way, and we can use the same test logic for both
----------------------------------------------------------------
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 #6285: Improve performance for distinct queries
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #6285:
URL: https://github.com/apache/incubator-pinot/pull/6285#discussion_r533690833
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/DistinctQueriesTest.java
##########
@@ -394,285 +614,273 @@ private DistinctTable getDistinctTableInnerSegment(String query, boolean isPql)
* </li>
* </ul>
*/
- private void testDistinctInterSegmentHelper(String[] pqlQueries, String[] sqlQueries)
- throws Exception {
- ImmutableSegment segment0 = createSegment(0, generateRecords(0));
- ImmutableSegment segment1 = createSegment(1, generateRecords(1000));
- _indexSegments = Arrays.asList(segment0, segment1);
- try {
- {
- // Test selecting all columns
- String pqlQuery = pqlQueries[0];
- String sqlQuery = sqlQueries[0];
-
- // Check data schema
- BrokerResponseNative pqlResponse = getBrokerResponseForPqlQuery(pqlQuery);
- SelectionResults selectionResults = pqlResponse.getSelectionResults();
- assertNotNull(selectionResults);
- assertEquals(selectionResults.getColumns(),
- Arrays.asList("intColumn", "longColumn", "floatColumn", "doubleColumn", "stringColumn", "bytesColumn"));
- BrokerResponseNative sqlResponse = getBrokerResponseForSqlQuery(sqlQuery);
- ResultTable resultTable = sqlResponse.getResultTable();
- assertNotNull(resultTable);
- DataSchema dataSchema = resultTable.getDataSchema();
- assertEquals(dataSchema.getColumnNames(),
- new String[]{"intColumn", "longColumn", "floatColumn", "doubleColumn", "stringColumn", "bytesColumn"});
- assertEquals(dataSchema.getColumnDataTypes(),
- new ColumnDataType[]{ColumnDataType.INT, ColumnDataType.LONG, ColumnDataType.FLOAT, ColumnDataType.DOUBLE, ColumnDataType.STRING, ColumnDataType.BYTES});
-
- // Check values, where all 200 unique values should be returned
- List<Serializable[]> pqlRows = selectionResults.getRows();
- assertEquals(pqlRows.size(), 2 * NUM_UNIQUE_RECORDS_PER_SEGMENT);
- List<Object[]> sqlRows = resultTable.getRows();
- assertEquals(sqlRows.size(), 2 * NUM_UNIQUE_RECORDS_PER_SEGMENT);
- Set<Integer> expectedValues = new HashSet<>();
- for (int i = 0; i < NUM_UNIQUE_RECORDS_PER_SEGMENT; i++) {
+ private void testDistinctInterSegmentHelper(String[] pqlQueries, String[] sqlQueries) {
+ {
+ // Test selecting all columns
+ String pqlQuery = pqlQueries[0];
+ String sqlQuery = sqlQueries[0];
+
+ // Check data schema
+ BrokerResponseNative pqlResponse = getBrokerResponseForPqlQuery(pqlQuery);
Review comment:
Unrelated to this PR - Can we please add another version of tests for SQL? Or may be just use `getBrokerResponseForSqlQuery()` instead of both PQL and SQL?
----------------------------------------------------------------
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