You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/02/08 23:14:25 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Jackie-Jiang opened a new pull request #8172:
URL: https://github.com/apache/pinot/pull/8172
When a query has the same mixed regular aggregation and filtered aggregation, the index of the aggregation function is not maintained correctly.
This PR contains the following changes:
- Fix `QueryContext` to maintain the correct index
- Fix `PostAggregationHandler` to always use the filtered aggregation index map
- Add tests for `QueryContext` and queries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r806164875
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
I'd suggest doing that when adding the FILTER support for group-by queries because it does some overhead (looking up pair vs function context)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r804969495
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
Review comment:
Ah I see. My bad, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r806536248
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -296,6 +296,19 @@ public void testMultipleAggregationsOnSameFilter() {
+ "MAX(CASE WHEN (INT_COL > 29990) THEN INT_COL ELSE 0 END) AS total_max, "
+ "SUM(CASE WHEN (NO_INDEX_COL < 5000) THEN INT_COL ELSE 0 END) AS total_sum, "
+ "MAX(CASE WHEN (NO_INDEX_COL < 5000) THEN NO_INDEX_COL ELSE 0 END) AS total_max2 FROM MyTable";
+ }
+
+ @Test
+ public void testMixedAggregations() {
Review comment:
Lets rename this to testMixedAggregationsOfSameType, since other tests in this class contain tests with mixed aggregations, just of different types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r804970313
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
Let's add that as a follow up? I feel it would be confusing for future readers if this class otherwise
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803106146
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/PostAggregationHandler.java
##########
@@ -117,19 +115,19 @@ public ValueExtractor getValueExtractor(ExpressionContext expression) {
}
}
FunctionContext function = expression.getFunction();
- Preconditions
- .checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause", expression);
+ Preconditions.checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause",
Review comment:
Separate all non-functional changes into #8184. Same for other whitespace related 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1035562142
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8172](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d75e78e) into [master](https://codecov.io/gh/apache/pinot/commit/b404e4adf15c897321ef62b9e986af1ae7e946dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b404e4a) will **decrease** coverage by `43.73%`.
> The diff coverage is `77.77%`.
> :exclamation: Current head d75e78e differs from pull request most recent head f7e5b0e. Consider uploading reports for the commit f7e5b0e to get more accurate results
[](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8172 +/- ##
=============================================
- Coverage 71.33% 27.60% -43.74%
=============================================
Files 1623 1612 -11
Lines 84314 83952 -362
Branches 12640 12600 -40
=============================================
- Hits 60146 23174 -36972
- Misses 20047 58620 +38573
+ Partials 4121 2158 -1963
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `27.60% <77.77%> (+0.13%)` | :arrow_up: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rg/apache/pinot/core/plan/AggregationPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0FnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `51.00% <0.00%> (-39.91%)` | :arrow_down: |
| [...inot/core/query/reduce/PostAggregationHandler.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUG9zdEFnZ3JlZ2F0aW9uSGFuZGxlci5qYXZh) | `84.70% <22.22%> (-7.16%)` | :arrow_down: |
| [...pinot/core/query/request/context/QueryContext.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvUXVlcnlDb250ZXh0LmphdmE=) | `92.38% <93.02%> (-5.20%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/BigDecimalUtils.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQmlnRGVjaW1hbFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1176 more](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a899110...f7e5b0e](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #8172:
URL: https://github.com/apache/pinot/pull/8172
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r802384374
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/PostAggregationHandler.java
##########
@@ -117,19 +115,19 @@ public ValueExtractor getValueExtractor(ExpressionContext expression) {
}
}
FunctionContext function = expression.getFunction();
- Preconditions
- .checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause", expression);
+ Preconditions.checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause",
Review comment:
Whitespace changes?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
+ }
+ queryContext._aggregationFunctions = aggregationFunctions;
+ queryContext._filteredAggregationFunctions = filteredAggregationFunctions;
queryContext._aggregationFunctionIndexMap = aggregationFunctionIndexMap;
- queryContext._filteredAggregationsIndexMap = filterExpressionIndexMap;
+ queryContext._filteredAggregationsIndexMap = filteredAggregationsIndexMap;
}
}
/**
- * Helper method to extract AGGREGATION FunctionContexts from the given expression.
- *
- * NOTE: The left pair of aggregations should be set only for filtered aggregations
+ * Helper method to extract AGGREGATION FunctionContexts and FILTER FilterContexts from the given expression.
*/
private static void getAggregations(ExpressionContext expression,
- List<Pair<FilterContext, FunctionContext>> aggregations) {
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations) {
Review comment:
Same comment as above
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -234,11 +233,11 @@ public BrokerRequest getBrokerRequest() {
}
/**
- * Returns the filtered aggregations for a query
+ * Returns the filtered aggregation functions for a query, or {@code null} if the query does not have any aggregation.
*/
@Nullable
- public List<Pair<AggregationFunction, FilterContext>> getFilteredAggregations() {
- return _filteredAggregations;
+ public List<Pair<AggregationFunction, FilterContext>> getFilteredAggregationFunctions() {
Review comment:
Renaming change?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
+ }
+ queryContext._aggregationFunctions = aggregationFunctions;
+ queryContext._filteredAggregationFunctions = filteredAggregationFunctions;
queryContext._aggregationFunctionIndexMap = aggregationFunctionIndexMap;
- queryContext._filteredAggregationsIndexMap = filterExpressionIndexMap;
+ queryContext._filteredAggregationsIndexMap = filteredAggregationsIndexMap;
}
}
/**
- * Helper method to extract AGGREGATION FunctionContexts from the given expression.
- *
- * NOTE: The left pair of aggregations should be set only for filtered aggregations
+ * Helper method to extract AGGREGATION FunctionContexts and FILTER FilterContexts from the given expression.
*/
private static void getAggregations(ExpressionContext expression,
- List<Pair<FilterContext, FunctionContext>> aggregations) {
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations) {
FunctionContext function = expression.getFunction();
if (function == null) {
return;
}
if (function.getType() == FunctionContext.Type.AGGREGATION) {
// Aggregation
- aggregations.add(Pair.of(null, function));
+ filteredAggregations.add(Pair.of(function, null));
} else {
- List<ExpressionContext> arguments = function.getArguments();
if (function.getFunctionName().equalsIgnoreCase("filter")) {
// Filtered aggregation
+ List<ExpressionContext> arguments = function.getArguments();
Review comment:
Where are we using this declaration?
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -143,287 +139,172 @@ private void buildSegment(String segmentName)
}
}
- private void testInterSegmentAggregationQueryHelper(String firstQuery, String secondQuery) {
- // SQL
- BrokerResponseNative firstBrokerResponseNative = getBrokerResponseForSqlQuery(firstQuery);
- BrokerResponseNative secondBrokerResponseNative = getBrokerResponseForSqlQuery(secondQuery);
- ResultTable firstResultTable = firstBrokerResponseNative.getResultTable();
- ResultTable secondResultTable = secondBrokerResponseNative.getResultTable();
- DataSchema firstDataSchema = firstResultTable.getDataSchema();
- DataSchema secondDataSchema = secondResultTable.getDataSchema();
-
- Assert.assertEquals(firstDataSchema.size(), secondDataSchema.size());
-
- List<Object[]> firstSetOfRows = firstResultTable.getRows();
- List<Object[]> secondSetOfRows = secondResultTable.getRows();
-
- Assert.assertEquals(firstSetOfRows.size(), secondSetOfRows.size());
-
- for (int i = 0; i < firstSetOfRows.size(); i++) {
- Object[] firstSetRow = firstSetOfRows.get(i);
- Object[] secondSetRow = secondSetOfRows.get(i);
-
- Assert.assertEquals(firstSetRow.length, secondSetRow.length);
-
- for (int j = 0; j < firstSetRow.length; j++) {
- //System.out.println("FIRST " + firstSetRow[j] + " SECOND " + secondSetRow[j] + " j " + j);
- Assert.assertEquals(firstSetRow[j], secondSetRow[j]);
- }
+ private void testQuery(String filterQuery, String nonFilterQuery) {
+ List<Object[]> filterQueryResults = getBrokerResponseForSqlQuery(filterQuery).getResultTable().getRows();
+ List<Object[]> nonFilterQueryResults = getBrokerResponseForSqlQuery(nonFilterQuery).getResultTable().getRows();
+ assertEquals(filterQueryResults.size(), nonFilterQueryResults.size());
+ for (int i = 0; i < filterQueryResults.size(); i++) {
+ assertEquals(filterQueryResults.get(i), nonFilterQueryResults.get(i));
}
}
@Test
- public void testInterSegment() {
-
- String query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 9999)"
- + "FROM MyTable WHERE INT_COL < 1000000";
-
- String nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 9999 AND INT_COL < 1000000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 1234 AND INT_COL < 22000)"
- + "FROM MyTable";
-
- nonFilterQuery = "SELECT SUM(CASE "
- + "WHEN (INT_COL > 1234 AND INT_COL < 22000) THEN INT_COL ELSE 0 "
- + "END) AS total_sum FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL < 3)"
- + "FROM MyTable WHERE INT_COL > 1";
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 1 AND INT_COL < 3";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT COUNT(*) FILTER(WHERE INT_COL = 4)"
- + "FROM MyTable";
- nonFilterQuery =
- "SELECT COUNT(*)"
- + "FROM MyTable WHERE INT_COL = 4";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 8000)"
- + "FROM MyTable ";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 8000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE NO_INDEX_COL <= 1)"
- + "FROM MyTable WHERE INT_COL > 1";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL <= 1 AND INT_COL > 1";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > -1";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),SUM(NO_INDEX_COL),MAX(INT_COL) "
- + "FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,SUM(NO_INDEX_COL),"
- + "MAX(INT_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(INT_COL) FILTER(WHERE NO_INDEX_COL > -1) FROM MyTable";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL) FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
+ public void testSimpleQueries() {
+ String filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 9999) FROM MyTable WHERE INT_COL < 1000000";
+ String nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 9999 AND INT_COL < 1000000";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL < 3) FROM MyTable WHERE INT_COL > 1";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 1 AND INT_COL < 3";
+ testQuery(filterQuery, nonFilterQuery);
- query =
- "SELECT MAX(INT_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ filterQuery = "SELECT COUNT(*) FILTER(WHERE INT_COL = 4) FROM MyTable";
+ nonFilterQuery = "SELECT COUNT(*) FROM MyTable WHERE INT_COL = 4";
+ testQuery(filterQuery, nonFilterQuery);
- nonFilterQuery =
- "SELECT MAX(CASE WHEN (INT_COL < 100) THEN INT_COL ELSE 0 END) AS total_max FROM MyTable";
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 8000) FROM MyTable ";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 8000";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE NO_INDEX_COL <= 1) FROM MyTable WHERE INT_COL > 1";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE NO_INDEX_COL <= 1 AND INT_COL > 1";
+ testQuery(filterQuery, nonFilterQuery);
- query =
- "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ filterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable WHERE NO_INDEX_COL > -1";
+ nonFilterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
- nonFilterQuery =
- "SELECT MIN(CASE WHEN (INT_COL < 100) THEN NO_INDEX_COL ELSE 0 END) AS total_min "
- + "FROM MyTable";
+ filterQuery = "SELECT AVG(INT_COL) FILTER(WHERE NO_INDEX_COL > -1) FROM MyTable";
+ nonFilterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL > 29990),MAX(INT_COL) FILTER(WHERE INT_COL > 29990)"
- + "FROM MyTable";
-
- nonFilterQuery =
- "SELECT MIN(NO_INDEX_COL), MAX(INT_COL) FROM MyTable WHERE INT_COL > 29990";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL > 29990), MAX(INT_COL) FILTER(WHERE INT_COL > 29990) "
+ + "FROM MyTable";
+ nonFilterQuery = "SELECT MIN(NO_INDEX_COL), MAX(INT_COL) FROM MyTable WHERE INT_COL > 29990";
+ testQuery(filterQuery, nonFilterQuery);
}
@Test
- public void testCaseVsFilter() {
- String query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 3),"
- + "SUM(INT_COL) FILTER(WHERE INT_COL < 4)"
- + "FROM MyTable WHERE INT_COL > 2";
-
- String nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 3) THEN INT_COL ELSE 0 "
- + "END) AS total_sum,SUM(CASE WHEN (INT_COL < 4) THEN INT_COL ELSE 0 END) AS total_sum2 "
+ public void testFilterVsCase() {
+ String filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 1234 AND INT_COL < 22000) FROM MyTable";
+ String nonFilterQuery =
+ "SELECT SUM(CASE WHEN (INT_COL > 1234 AND INT_COL < 22000) THEN INT_COL ELSE 0 END) AS total_sum FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0), SUM(NO_INDEX_COL), MAX(INT_COL) FROM MyTable";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(NO_INDEX_COL), MAX(INT_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0), MAX(NO_INDEX_COL) FROM MyTable";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "MAX(NO_INDEX_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery =
+ "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL) FROM MyTable WHERE NO_INDEX_COL > 5";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "MAX(NO_INDEX_COL) FROM MyTable WHERE NO_INDEX_COL > 5";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT MAX(INT_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ nonFilterQuery = "SELECT MAX(CASE WHEN (INT_COL < 100) THEN INT_COL ELSE 0 END) AS total_max FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ nonFilterQuery = "SELECT MIN(CASE WHEN (INT_COL < 100) THEN NO_INDEX_COL ELSE 0 END) AS total_min FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 3), SUM(INT_COL) FILTER(WHERE INT_COL < 4) "
+ "FROM MyTable WHERE INT_COL > 2";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 3) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(CASE WHEN (INT_COL < 4) THEN INT_COL ELSE 0 END) AS total_sum2 FROM MyTable WHERE INT_COL > 2";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345),SUM(INT_COL) FILTER(WHERE INT_COL < 59999),"
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345), SUM(INT_COL) FILTER(WHERE INT_COL < 59999), "
+ "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) FROM MyTable WHERE INT_COL > 1000";
-
- nonFilterQuery = "SELECT SUM( CASE WHEN (INT_COL > 12345) THEN INT_COL ELSE 0 "
- + "END) AS total_sum,SUM(CASE WHEN (INT_COL < 59999) THEN INT_COL ELSE 0 "
- + "END) AS total_sum2,MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL "
- + "ELSE 9999999 END) AS total_min FROM MyTable WHERE INT_COL > 1000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345),"
- + "SUM(NO_INDEX_COL) FILTER(WHERE INT_COL < 59999),"
- + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) "
+ nonFilterQuery = "SELECT SUM( CASE WHEN (INT_COL > 12345) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(CASE WHEN (INT_COL < 59999) THEN INT_COL ELSE 0 END) AS total_sum2, "
+ + "MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL ELSE 9999999 END) AS total_min "
+ "FROM MyTable WHERE INT_COL > 1000";
+ testQuery(filterQuery, nonFilterQuery);
- nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 12345) THEN INT_COL "
- + "ELSE 0 END) AS total_sum,SUM(CASE WHEN (INT_COL < 59999) THEN NO_INDEX_COL "
- + "ELSE 0 END) AS total_sum2,MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL "
- + "ELSE 9999999 END) AS total_min FROM MyTable WHERE INT_COL > 1000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345),"
- + "SUM(NO_INDEX_COL) FILTER(WHERE INT_COL < 59999),"
- + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) "
- + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000 ";
-
- nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 12345) THEN INT_COL ELSE 0 "
- + "END) AS total_sum,SUM(CASE WHEN (INT_COL < 59999) THEN NO_INDEX_COL "
- + "ELSE 0 END) AS total_sum2,MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL "
- + "ELSE 9999999 END) AS total_min FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE ABS(INT_COL) > 12345),"
- + "SUM(NO_INDEX_COL) FILTER(WHERE LN(INT_COL) < 59999),"
- + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) "
- + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000 ";
-
- nonFilterQuery = "SELECT SUM("
- + "CASE WHEN (ABS(INT_COL) > 12345) THEN INT_COL ELSE 0 "
- + "END) AS total_sum,SUM(CASE WHEN (LN(INT_COL) < 59999) THEN NO_INDEX_COL "
- + "ELSE 0 END) AS total_sum2,MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL "
- + "ELSE 9999999 END) AS total_min FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE MOD(INT_COL, STATIC_INT_COL) = 0),"
- + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) "
- + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000 ";
-
- nonFilterQuery = "SELECT SUM(CASE WHEN (MOD(INT_COL, STATIC_INT_COL) = 0) THEN INT_COL "
- + "ELSE 0 END) AS total_sum,MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL "
- + "ELSE 9999999 END) AS total_min FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 123 AND INT_COL < 25000),"
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345), SUM(NO_INDEX_COL) FILTER(WHERE INT_COL < 59999), "
+ + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) FROM MyTable WHERE INT_COL > 1000";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 12345) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(CASE WHEN (INT_COL < 59999) THEN NO_INDEX_COL ELSE 0 END) AS total_sum2, "
+ + "MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL ELSE 9999999 END) AS total_min "
+ + "FROM MyTable WHERE INT_COL > 1000";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 12345), SUM(NO_INDEX_COL) FILTER(WHERE INT_COL < 59999), "
+ + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 12345) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(CASE WHEN (INT_COL < 59999) THEN NO_INDEX_COL ELSE 0 END) AS total_sum2, "
+ + "MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL ELSE 9999999 END) AS total_min "
+ + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE ABS(INT_COL) > 12345), "
+ + "SUM(NO_INDEX_COL) FILTER(WHERE LN(INT_COL) < 59999), MIN(INT_COL) FILTER(WHERE INT_COL > 5000) "
+ + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (ABS(INT_COL) > 12345) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "SUM(CASE WHEN (LN(INT_COL) < 59999) THEN NO_INDEX_COL ELSE 0 END) AS total_sum2, "
+ + "MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL ELSE 9999999 END) AS total_min "
+ + "FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
+ testQuery(filterQuery, nonFilterQuery);
+
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE MOD(INT_COL, STATIC_INT_COL) = 0), "
+ + "MIN(INT_COL) FILTER(WHERE INT_COL > 5000) FROM MyTable WHERE INT_COL < 28000 AND NO_INDEX_COL > 3000";
+ nonFilterQuery = "SELECT SUM(CASE WHEN (MOD(INT_COL, STATIC_INT_COL) = 0) THEN INT_COL ELSE 0 END) AS total_sum, "
+ + "MIN(CASE WHEN (INT_COL > 5000) THEN INT_COL ELSE 9999999 END) AS total_min "
Review comment:
The changes in this file predominantly seem to be oriented around whitespacing and/or test function name changes. Suggest isolating changes relevant to the main changes and performing whitespace related changes in a separate PR.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -90,12 +90,11 @@
// Pre-calculate the aggregation functions and columns for the query so that it can be shared across all the segments
private AggregationFunction[] _aggregationFunctions;
+ private List<Pair<AggregationFunction, FilterContext>> _filteredAggregationFunctions;
Review comment:
Same comment as above -- renaming change?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
+ }
+ queryContext._aggregationFunctions = aggregationFunctions;
+ queryContext._filteredAggregationFunctions = filteredAggregationFunctions;
queryContext._aggregationFunctionIndexMap = aggregationFunctionIndexMap;
- queryContext._filteredAggregationsIndexMap = filterExpressionIndexMap;
+ queryContext._filteredAggregationsIndexMap = filteredAggregationsIndexMap;
}
}
/**
- * Helper method to extract AGGREGATION FunctionContexts from the given expression.
- *
- * NOTE: The left pair of aggregations should be set only for filtered aggregations
+ * Helper method to extract AGGREGATION FunctionContexts and FILTER FilterContexts from the given expression.
*/
private static void getAggregations(ExpressionContext expression,
- List<Pair<FilterContext, FunctionContext>> aggregations) {
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations) {
FunctionContext function = expression.getFunction();
if (function == null) {
return;
}
if (function.getType() == FunctionContext.Type.AGGREGATION) {
// Aggregation
- aggregations.add(Pair.of(null, function));
+ filteredAggregations.add(Pair.of(function, null));
Review comment:
Naming change? Also relates to my original question around need to change the order of the pair.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
Review comment:
Renaming changes?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -90,12 +90,11 @@
// Pre-calculate the aggregation functions and columns for the query so that it can be shared across all the segments
private AggregationFunction[] _aggregationFunctions;
+ private List<Pair<AggregationFunction, FilterContext>> _filteredAggregationFunctions;
- private List<Pair<AggregationFunction, FilterContext>> _filteredAggregations;
-
+ private Map<FunctionContext, Integer> _aggregationFunctionIndexMap;
Review comment:
Unintended relocation of declaration?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
Is aggregationFunctionIndexMap really needed anymore?
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -106,27 +104,25 @@ public void tearDown() {
FileUtils.deleteQuietly(INDEX_DIR);
}
- private List<GenericRow> createTestData(int numRows) {
- List<GenericRow> rows = new ArrayList<>();
-
- for (int i = 0; i < numRows; i++) {
+ private List<GenericRow> createTestData() {
+ List<GenericRow> rows = new ArrayList<>(NUM_ROWS);
+ for (int i = 0; i < NUM_ROWS; i++) {
GenericRow row = new GenericRow();
- row.putField(INT_COL_NAME, INT_BASE_VALUE + i);
- row.putField(NO_INDEX_INT_COL_NAME, i);
- row.putField(STATIC_INT_COL_NAME, 10);
-
+ row.putValue(INT_COL_NAME, INT_BASE_VALUE + i);
+ row.putValue(NO_INDEX_INT_COL_NAME, i);
Review comment:
Whitespace changes?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
Review comment:
I am not sure about this change. In the original PR, the initial implementation had `Pair<FunctionContext, FilterContext>`, but you (rightly) suggested that we do FilterContext first to follow convention. Also, looking at the usage of the same below, there seems to be no difference in using the prior or the later. Am I missing something, please?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/PostAggregationHandler.java
##########
@@ -117,19 +115,19 @@ public ValueExtractor getValueExtractor(ExpressionContext expression) {
}
}
FunctionContext function = expression.getFunction();
- Preconditions
- .checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause", expression);
+ Preconditions.checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause",
+ expression);
if (function.getType() == FunctionContext.Type.AGGREGATION) {
// Aggregation function
- return new ColumnValueExtractor(_aggregationFunctionIndexMap.get(function) + _numGroupByExpressions);
- } else if (function.getType() == FunctionContext.Type.TRANSFORM
- && function.getFunctionName().equalsIgnoreCase("filter")) {
+ return new ColumnValueExtractor(
+ _filteredAggregationsIndexMap.get(Pair.of(function, null)) + _numGroupByExpressions);
Review comment:
The main change here is that regular aggregations also refer to the filtered aggregation functions index map, instead of the standard index map. The remaining changes seem whitespace/naming changes?
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -143,287 +139,172 @@ private void buildSegment(String segmentName)
}
}
- private void testInterSegmentAggregationQueryHelper(String firstQuery, String secondQuery) {
- // SQL
- BrokerResponseNative firstBrokerResponseNative = getBrokerResponseForSqlQuery(firstQuery);
- BrokerResponseNative secondBrokerResponseNative = getBrokerResponseForSqlQuery(secondQuery);
- ResultTable firstResultTable = firstBrokerResponseNative.getResultTable();
- ResultTable secondResultTable = secondBrokerResponseNative.getResultTable();
- DataSchema firstDataSchema = firstResultTable.getDataSchema();
- DataSchema secondDataSchema = secondResultTable.getDataSchema();
-
- Assert.assertEquals(firstDataSchema.size(), secondDataSchema.size());
-
- List<Object[]> firstSetOfRows = firstResultTable.getRows();
- List<Object[]> secondSetOfRows = secondResultTable.getRows();
-
- Assert.assertEquals(firstSetOfRows.size(), secondSetOfRows.size());
-
- for (int i = 0; i < firstSetOfRows.size(); i++) {
- Object[] firstSetRow = firstSetOfRows.get(i);
- Object[] secondSetRow = secondSetOfRows.get(i);
-
- Assert.assertEquals(firstSetRow.length, secondSetRow.length);
-
- for (int j = 0; j < firstSetRow.length; j++) {
- //System.out.println("FIRST " + firstSetRow[j] + " SECOND " + secondSetRow[j] + " j " + j);
- Assert.assertEquals(firstSetRow[j], secondSetRow[j]);
- }
+ private void testQuery(String filterQuery, String nonFilterQuery) {
+ List<Object[]> filterQueryResults = getBrokerResponseForSqlQuery(filterQuery).getResultTable().getRows();
+ List<Object[]> nonFilterQueryResults = getBrokerResponseForSqlQuery(nonFilterQuery).getResultTable().getRows();
+ assertEquals(filterQueryResults.size(), nonFilterQueryResults.size());
+ for (int i = 0; i < filterQueryResults.size(); i++) {
+ assertEquals(filterQueryResults.get(i), nonFilterQueryResults.get(i));
}
}
@Test
- public void testInterSegment() {
-
- String query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 9999)"
- + "FROM MyTable WHERE INT_COL < 1000000";
-
- String nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 9999 AND INT_COL < 1000000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 1234 AND INT_COL < 22000)"
- + "FROM MyTable";
-
- nonFilterQuery = "SELECT SUM(CASE "
- + "WHEN (INT_COL > 1234 AND INT_COL < 22000) THEN INT_COL ELSE 0 "
- + "END) AS total_sum FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL < 3)"
- + "FROM MyTable WHERE INT_COL > 1";
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 1 AND INT_COL < 3";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT COUNT(*) FILTER(WHERE INT_COL = 4)"
- + "FROM MyTable";
- nonFilterQuery =
- "SELECT COUNT(*)"
- + "FROM MyTable WHERE INT_COL = 4";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 8000)"
- + "FROM MyTable ";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 8000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE NO_INDEX_COL <= 1)"
- + "FROM MyTable WHERE INT_COL > 1";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL <= 1 AND INT_COL > 1";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > -1";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),SUM(NO_INDEX_COL),MAX(INT_COL) "
- + "FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,SUM(NO_INDEX_COL),"
- + "MAX(INT_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(INT_COL) FILTER(WHERE NO_INDEX_COL > -1) FROM MyTable";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL) FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
+ public void testSimpleQueries() {
Review comment:
Naming + whitespace changes?
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -143,287 +139,172 @@ private void buildSegment(String segmentName)
}
}
- private void testInterSegmentAggregationQueryHelper(String firstQuery, String secondQuery) {
- // SQL
- BrokerResponseNative firstBrokerResponseNative = getBrokerResponseForSqlQuery(firstQuery);
- BrokerResponseNative secondBrokerResponseNative = getBrokerResponseForSqlQuery(secondQuery);
- ResultTable firstResultTable = firstBrokerResponseNative.getResultTable();
- ResultTable secondResultTable = secondBrokerResponseNative.getResultTable();
- DataSchema firstDataSchema = firstResultTable.getDataSchema();
- DataSchema secondDataSchema = secondResultTable.getDataSchema();
-
- Assert.assertEquals(firstDataSchema.size(), secondDataSchema.size());
-
- List<Object[]> firstSetOfRows = firstResultTable.getRows();
- List<Object[]> secondSetOfRows = secondResultTable.getRows();
-
- Assert.assertEquals(firstSetOfRows.size(), secondSetOfRows.size());
-
- for (int i = 0; i < firstSetOfRows.size(); i++) {
- Object[] firstSetRow = firstSetOfRows.get(i);
- Object[] secondSetRow = secondSetOfRows.get(i);
-
- Assert.assertEquals(firstSetRow.length, secondSetRow.length);
-
- for (int j = 0; j < firstSetRow.length; j++) {
- //System.out.println("FIRST " + firstSetRow[j] + " SECOND " + secondSetRow[j] + " j " + j);
- Assert.assertEquals(firstSetRow[j], secondSetRow[j]);
- }
+ private void testQuery(String filterQuery, String nonFilterQuery) {
+ List<Object[]> filterQueryResults = getBrokerResponseForSqlQuery(filterQuery).getResultTable().getRows();
Review comment:
I am not sure if this method has the same intent as the original method -- the original method had extra checks around row lengths and number of rows, and the result schemas. Can we please revert this? This anyways does not seem related to the problem the PR is solving.
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -143,287 +139,172 @@ private void buildSegment(String segmentName)
}
}
- private void testInterSegmentAggregationQueryHelper(String firstQuery, String secondQuery) {
- // SQL
- BrokerResponseNative firstBrokerResponseNative = getBrokerResponseForSqlQuery(firstQuery);
- BrokerResponseNative secondBrokerResponseNative = getBrokerResponseForSqlQuery(secondQuery);
- ResultTable firstResultTable = firstBrokerResponseNative.getResultTable();
- ResultTable secondResultTable = secondBrokerResponseNative.getResultTable();
- DataSchema firstDataSchema = firstResultTable.getDataSchema();
- DataSchema secondDataSchema = secondResultTable.getDataSchema();
-
- Assert.assertEquals(firstDataSchema.size(), secondDataSchema.size());
-
- List<Object[]> firstSetOfRows = firstResultTable.getRows();
- List<Object[]> secondSetOfRows = secondResultTable.getRows();
-
- Assert.assertEquals(firstSetOfRows.size(), secondSetOfRows.size());
-
- for (int i = 0; i < firstSetOfRows.size(); i++) {
- Object[] firstSetRow = firstSetOfRows.get(i);
- Object[] secondSetRow = secondSetOfRows.get(i);
-
- Assert.assertEquals(firstSetRow.length, secondSetRow.length);
-
- for (int j = 0; j < firstSetRow.length; j++) {
- //System.out.println("FIRST " + firstSetRow[j] + " SECOND " + secondSetRow[j] + " j " + j);
- Assert.assertEquals(firstSetRow[j], secondSetRow[j]);
- }
+ private void testQuery(String filterQuery, String nonFilterQuery) {
+ List<Object[]> filterQueryResults = getBrokerResponseForSqlQuery(filterQuery).getResultTable().getRows();
+ List<Object[]> nonFilterQueryResults = getBrokerResponseForSqlQuery(nonFilterQuery).getResultTable().getRows();
+ assertEquals(filterQueryResults.size(), nonFilterQueryResults.size());
+ for (int i = 0; i < filterQueryResults.size(); i++) {
+ assertEquals(filterQueryResults.get(i), nonFilterQueryResults.get(i));
}
}
@Test
- public void testInterSegment() {
-
- String query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 9999)"
- + "FROM MyTable WHERE INT_COL < 1000000";
-
- String nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 9999 AND INT_COL < 1000000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 1234 AND INT_COL < 22000)"
- + "FROM MyTable";
-
- nonFilterQuery = "SELECT SUM(CASE "
- + "WHEN (INT_COL > 1234 AND INT_COL < 22000) THEN INT_COL ELSE 0 "
- + "END) AS total_sum FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL < 3)"
- + "FROM MyTable WHERE INT_COL > 1";
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 1 AND INT_COL < 3";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT COUNT(*) FILTER(WHERE INT_COL = 4)"
- + "FROM MyTable";
- nonFilterQuery =
- "SELECT COUNT(*)"
- + "FROM MyTable WHERE INT_COL = 4";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 8000)"
- + "FROM MyTable ";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE INT_COL > 8000";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE NO_INDEX_COL <= 1)"
- + "FROM MyTable WHERE INT_COL > 1";
-
- nonFilterQuery =
- "SELECT SUM(INT_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL <= 1 AND INT_COL > 1";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > -1";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),SUM(NO_INDEX_COL),MAX(INT_COL) "
- + "FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,SUM(NO_INDEX_COL),"
- + "MAX(INT_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT AVG(INT_COL) FILTER(WHERE NO_INDEX_COL > -1) FROM MyTable";
- nonFilterQuery =
- "SELECT AVG(NO_INDEX_COL) FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL) FROM MyTable";
-
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT SUM(INT_COL) FILTER(WHERE INT_COL % 10 = 0),MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
- nonFilterQuery =
- "SELECT SUM(CASE WHEN (INT_COL % 10 = 0) THEN INT_COL ELSE 0 END) AS total_sum,MAX(NO_INDEX_COL)"
- + "FROM MyTable WHERE NO_INDEX_COL > 5";
+ public void testSimpleQueries() {
+ String filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 9999) FROM MyTable WHERE INT_COL < 1000000";
+ String nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 9999 AND INT_COL < 1000000";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL < 3) FROM MyTable WHERE INT_COL > 1";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 1 AND INT_COL < 3";
+ testQuery(filterQuery, nonFilterQuery);
- query =
- "SELECT MAX(INT_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ filterQuery = "SELECT COUNT(*) FILTER(WHERE INT_COL = 4) FROM MyTable";
+ nonFilterQuery = "SELECT COUNT(*) FROM MyTable WHERE INT_COL = 4";
+ testQuery(filterQuery, nonFilterQuery);
- nonFilterQuery =
- "SELECT MAX(CASE WHEN (INT_COL < 100) THEN INT_COL ELSE 0 END) AS total_max FROM MyTable";
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 8000) FROM MyTable ";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE INT_COL > 8000";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT SUM(INT_COL) FILTER(WHERE NO_INDEX_COL <= 1) FROM MyTable WHERE INT_COL > 1";
+ nonFilterQuery = "SELECT SUM(INT_COL) FROM MyTable WHERE NO_INDEX_COL <= 1 AND INT_COL > 1";
+ testQuery(filterQuery, nonFilterQuery);
- query =
- "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL < 100) FROM MyTable";
+ filterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable WHERE NO_INDEX_COL > -1";
+ nonFilterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
- nonFilterQuery =
- "SELECT MIN(CASE WHEN (INT_COL < 100) THEN NO_INDEX_COL ELSE 0 END) AS total_min "
- + "FROM MyTable";
+ filterQuery = "SELECT AVG(INT_COL) FILTER(WHERE NO_INDEX_COL > -1) FROM MyTable";
+ nonFilterQuery = "SELECT AVG(NO_INDEX_COL) FROM MyTable";
+ testQuery(filterQuery, nonFilterQuery);
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
-
- query =
- "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL > 29990),MAX(INT_COL) FILTER(WHERE INT_COL > 29990)"
- + "FROM MyTable";
-
- nonFilterQuery =
- "SELECT MIN(NO_INDEX_COL), MAX(INT_COL) FROM MyTable WHERE INT_COL > 29990";
-
- testInterSegmentAggregationQueryHelper(query, nonFilterQuery);
+ filterQuery = "SELECT MIN(NO_INDEX_COL) FILTER(WHERE INT_COL > 29990), MAX(INT_COL) FILTER(WHERE INT_COL > 29990) "
+ + "FROM MyTable";
+ nonFilterQuery = "SELECT MIN(NO_INDEX_COL), MAX(INT_COL) FROM MyTable WHERE INT_COL > 29990";
+ testQuery(filterQuery, nonFilterQuery);
}
@Test
- public void testCaseVsFilter() {
- String query = "SELECT SUM(INT_COL) FILTER(WHERE INT_COL > 3),"
- + "SUM(INT_COL) FILTER(WHERE INT_COL < 4)"
- + "FROM MyTable WHERE INT_COL > 2";
-
- String nonFilterQuery = "SELECT SUM(CASE WHEN (INT_COL > 3) THEN INT_COL ELSE 0 "
- + "END) AS total_sum,SUM(CASE WHEN (INT_COL < 4) THEN INT_COL ELSE 0 END) AS total_sum2 "
+ public void testFilterVsCase() {
Review comment:
Naming + whitespace changes?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang edited a comment on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang edited a comment on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1034035696
@atris Most of the whitespace changes are actually done by the IDE auto-formatting. Let me separate them into a separate PR so that the change for the bug fix is more clear.
The main fix is in 2 parts:
- Index calculation in QueryContext
- Index lookup in PostAggregationHandler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1035562142
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8172](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7e5b0e) into [master](https://codecov.io/gh/apache/pinot/commit/b404e4adf15c897321ef62b9e986af1ae7e946dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b404e4a) will **decrease** coverage by `0.96%`.
> The diff coverage is `94.44%`.
[](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8172 +/- ##
============================================
- Coverage 71.33% 70.37% -0.97%
Complexity 4303 4303
============================================
Files 1623 1623
Lines 84314 84304 -10
Branches 12640 12638 -2
============================================
- Hits 60146 59328 -818
- Misses 20047 20877 +830
+ Partials 4121 4099 -22
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.86% <77.77%> (-0.02%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `67.88% <94.44%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.19% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/core/query/reduce/PostAggregationHandler.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUG9zdEFnZ3JlZ2F0aW9uSGFuZGxlci5qYXZh) | `91.76% <66.66%> (-0.10%)` | :arrow_down: |
| [...rg/apache/pinot/core/plan/AggregationPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0FnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `91.00% <100.00%> (+0.09%)` | :arrow_up: |
| [...pinot/core/query/request/context/QueryContext.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvUXVlcnlDb250ZXh0LmphdmE=) | `97.96% <100.00%> (+0.38%)` | :arrow_up: |
| [...t/core/plan/StreamingInstanceResponsePlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ0luc3RhbmNlUmVzcG9uc2VQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...he/pinot/core/plan/StreamingSelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...ator/streaming/StreamingSelectionOnlyOperator.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-87.81%)` | :arrow_down: |
| [...re/query/reduce/SelectionOnlyStreamingReducer.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU2VsZWN0aW9uT25seVN0cmVhbWluZ1JlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| ... and [92 more](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a899110...f7e5b0e](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1034035696
@atris Most of the whitespace changes are actually done by the IDE auto-formatting. Let me separate them into a separate PR so that the change for the bug fix is more clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1035562142
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8172](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7e5b0e) into [master](https://codecov.io/gh/apache/pinot/commit/b404e4adf15c897321ef62b9e986af1ae7e946dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b404e4a) will **decrease** coverage by `0.97%`.
> The diff coverage is `94.44%`.
[](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8172 +/- ##
============================================
- Coverage 71.33% 70.35% -0.98%
Complexity 4303 4303
============================================
Files 1623 1623
Lines 84314 84304 -10
Branches 12640 12638 -2
============================================
- Hits 60146 59316 -830
- Misses 20047 20886 +839
+ Partials 4121 4102 -19
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.86% <77.77%> (-0.02%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `67.88% <94.44%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.17% <0.00%> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/core/query/reduce/PostAggregationHandler.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUG9zdEFnZ3JlZ2F0aW9uSGFuZGxlci5qYXZh) | `91.76% <66.66%> (-0.10%)` | :arrow_down: |
| [...rg/apache/pinot/core/plan/AggregationPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0FnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `91.00% <100.00%> (+0.09%)` | :arrow_up: |
| [...pinot/core/query/request/context/QueryContext.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvUXVlcnlDb250ZXh0LmphdmE=) | `97.96% <100.00%> (+0.38%)` | :arrow_up: |
| [...t/core/plan/StreamingInstanceResponsePlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ0luc3RhbmNlUmVzcG9uc2VQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...he/pinot/core/plan/StreamingSelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...ator/streaming/StreamingSelectionOnlyOperator.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-87.81%)` | :arrow_down: |
| [...re/query/reduce/SelectionOnlyStreamingReducer.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU2VsZWN0aW9uT25seVN0cmVhbWluZ1JlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| ... and [94 more](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a899110...f7e5b0e](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r806171344
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
Sure, I can submit a followup PR to remove it after merging this 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang edited a comment on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang edited a comment on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1034035696
@atris Most of the whitespace changes are actually done by the IDE auto-formatting. Let me separate them into a separate PR so that the change for the bug fix is more clear.
The main fix is in 2 parts:
- Index calculation in QueryContext
- Index lookup in PostAggregationHandler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803114460
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
+ }
+ queryContext._aggregationFunctions = aggregationFunctions;
+ queryContext._filteredAggregationFunctions = filteredAggregationFunctions;
queryContext._aggregationFunctionIndexMap = aggregationFunctionIndexMap;
- queryContext._filteredAggregationsIndexMap = filterExpressionIndexMap;
+ queryContext._filteredAggregationsIndexMap = filteredAggregationsIndexMap;
}
}
/**
- * Helper method to extract AGGREGATION FunctionContexts from the given expression.
- *
- * NOTE: The left pair of aggregations should be set only for filtered aggregations
+ * Helper method to extract AGGREGATION FunctionContexts and FILTER FilterContexts from the given expression.
*/
private static void getAggregations(ExpressionContext expression,
- List<Pair<FilterContext, FunctionContext>> aggregations) {
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations) {
FunctionContext function = expression.getFunction();
if (function == null) {
return;
}
if (function.getType() == FunctionContext.Type.AGGREGATION) {
// Aggregation
- aggregations.add(Pair.of(null, function));
+ filteredAggregations.add(Pair.of(function, null));
} else {
- List<ExpressionContext> arguments = function.getArguments();
if (function.getFunctionName().equalsIgnoreCase("filter")) {
// Filtered aggregation
+ List<ExpressionContext> arguments = function.getArguments();
Review comment:
Reverted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803112659
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
It is still used in `TableResizer` for group-by queries. We can potentially clean it up, but that is out of the scope of this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803109604
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
Review comment:
That is not true. In the original PR, initially we had filter in front of function, then I suggested changing it to have function in the front for readability (pair of <aggregation, filter>). All other places have been changed except for this one, so I changed it to be consistent in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803106146
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/reduce/PostAggregationHandler.java
##########
@@ -117,19 +115,19 @@ public ValueExtractor getValueExtractor(ExpressionContext expression) {
}
}
FunctionContext function = expression.getFunction();
- Preconditions
- .checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause", expression);
+ Preconditions.checkState(function != null, "Failed to find SELECT expression: %s in the GROUP-BY clause",
Review comment:
Separate all non-functional changes into #8184
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803116594
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -143,287 +139,172 @@ private void buildSegment(String segmentName)
}
}
- private void testInterSegmentAggregationQueryHelper(String firstQuery, String secondQuery) {
- // SQL
- BrokerResponseNative firstBrokerResponseNative = getBrokerResponseForSqlQuery(firstQuery);
- BrokerResponseNative secondBrokerResponseNative = getBrokerResponseForSqlQuery(secondQuery);
- ResultTable firstResultTable = firstBrokerResponseNative.getResultTable();
- ResultTable secondResultTable = secondBrokerResponseNative.getResultTable();
- DataSchema firstDataSchema = firstResultTable.getDataSchema();
- DataSchema secondDataSchema = secondResultTable.getDataSchema();
-
- Assert.assertEquals(firstDataSchema.size(), secondDataSchema.size());
-
- List<Object[]> firstSetOfRows = firstResultTable.getRows();
- List<Object[]> secondSetOfRows = secondResultTable.getRows();
-
- Assert.assertEquals(firstSetOfRows.size(), secondSetOfRows.size());
-
- for (int i = 0; i < firstSetOfRows.size(); i++) {
- Object[] firstSetRow = firstSetOfRows.get(i);
- Object[] secondSetRow = secondSetOfRows.get(i);
-
- Assert.assertEquals(firstSetRow.length, secondSetRow.length);
-
- for (int j = 0; j < firstSetRow.length; j++) {
- //System.out.println("FIRST " + firstSetRow[j] + " SECOND " + secondSetRow[j] + " j " + j);
- Assert.assertEquals(firstSetRow[j], secondSetRow[j]);
- }
+ private void testQuery(String filterQuery, String nonFilterQuery) {
+ List<Object[]> filterQueryResults = getBrokerResponseForSqlQuery(filterQuery).getResultTable().getRows();
Review comment:
Added the data schema check in #8184
The rows check remains the same, where `assertEquals(Object[] a, Object[] b)` will verify the array size and array content
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r806164875
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
I'd suggest doing that when adding the FILTER support for group-by queries because it does some overhead (looking up pair vs function context)
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
// Add aggregation functions in the SELECT clause
// NOTE: DO NOT deduplicate the aggregation functions in the SELECT clause because that involves protocol change.
- List<Pair<FilterContext, FunctionContext>> aggregationsInSelect = new ArrayList<>();
+ List<Pair<FunctionContext, FilterContext>> filteredAggregations = new ArrayList<>();
for (ExpressionContext selectExpression : queryContext._selectExpressions) {
- getAggregations(selectExpression, aggregationsInSelect);
+ getAggregations(selectExpression, filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInSelect) {
- FunctionContext function = pair.getRight();
- int functionIndex = filteredAggregations.size();
- AggregationFunction aggregationFunction =
- AggregationFunctionFactory.getAggregationFunction(function, queryContext);
-
- FilterContext filterContext = null;
- // If the left pair is not null, implies a filtered aggregation
- if (pair.getLeft() != null) {
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ if (filter != null) {
+ // Filtered aggregation
if (_groupByExpressions != null) {
throw new IllegalStateException("GROUP BY with FILTER clauses is not supported");
}
queryContext._hasFilteredAggregations = true;
- filterContext = pair.getLeft();
- Pair<FunctionContext, FilterContext> filterContextPair =
- Pair.of(function, filterContext);
- if (!filterExpressionIndexMap.containsKey(filterContextPair)) {
- int filterMapIndex = filterExpressionIndexMap.size();
- filterExpressionIndexMap.put(filterContextPair, filterMapIndex);
- }
}
- filteredAggregations.add(Pair.of(aggregationFunction, filterContext));
- aggregationFunctionIndexMap.put(function, functionIndex);
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
- // Add aggregation functions in the HAVING clause but not in the SELECT clause
+ // Add aggregation functions in the HAVING and ORDER-BY clause but not in the SELECT clause
+ filteredAggregations.clear();
if (queryContext._havingFilter != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInHaving = new ArrayList<>();
- getAggregations(queryContext._havingFilter, aggregationsInHaving);
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInHaving) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
- }
+ getAggregations(queryContext._havingFilter, filteredAggregations);
}
-
- // Add aggregation functions in the ORDER-BY clause but not in the SELECT or HAVING clause
if (queryContext._orderByExpressions != null) {
- List<Pair<FilterContext, FunctionContext>> aggregationsInOrderBy = new ArrayList<>();
for (OrderByExpressionContext orderByExpression : queryContext._orderByExpressions) {
- getAggregations(orderByExpression.getExpression(), aggregationsInOrderBy);
+ getAggregations(orderByExpression.getExpression(), filteredAggregations);
}
- for (Pair<FilterContext, FunctionContext> pair : aggregationsInOrderBy) {
- FunctionContext function = pair.getRight();
- if (!aggregationFunctionIndexMap.containsKey(function)) {
- int functionIndex = filteredAggregations.size();
- filteredAggregations.add(Pair.of(
- AggregationFunctionFactory.getAggregationFunction(function, queryContext), null));
- aggregationFunctionIndexMap.put(function, functionIndex);
- }
+ }
+ for (Pair<FunctionContext, FilterContext> pair : filteredAggregations) {
+ if (!filteredAggregationsIndexMap.containsKey(pair)) {
+ FunctionContext aggregation = pair.getLeft();
+ FilterContext filter = pair.getRight();
+ int functionIndex = filteredAggregationFunctions.size();
+ AggregationFunction aggregationFunction =
+ AggregationFunctionFactory.getAggregationFunction(aggregation, queryContext);
+ filteredAggregationFunctions.add(Pair.of(aggregationFunction, filter));
+ filteredAggregationsIndexMap.put(Pair.of(aggregation, filter), functionIndex);
}
}
- if (!filteredAggregations.isEmpty()) {
- for (Pair<AggregationFunction, FilterContext> pair : filteredAggregations) {
- aggregationFunctions.add(pair.getLeft());
+ if (!filteredAggregationFunctions.isEmpty()) {
+ int numAggregations = filteredAggregationFunctions.size();
+ AggregationFunction[] aggregationFunctions = new AggregationFunction[numAggregations];
+ for (int i = 0; i < numAggregations; i++) {
+ aggregationFunctions[i] = filteredAggregationFunctions.get(i).getLeft();
}
-
- queryContext._aggregationFunctions = aggregationFunctions.toArray(new AggregationFunction[0]);
- queryContext._filteredAggregations = filteredAggregations;
+ Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
+ for (Map.Entry<Pair<FunctionContext, FilterContext>, Integer> entry : filteredAggregationsIndexMap.entrySet()) {
+ aggregationFunctionIndexMap.put(entry.getKey().getLeft(), entry.getValue());
Review comment:
Sure, I can submit a followup PR to remove it after merging this one
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -296,6 +296,19 @@ public void testMultipleAggregationsOnSameFilter() {
+ "MAX(CASE WHEN (INT_COL > 29990) THEN INT_COL ELSE 0 END) AS total_max, "
+ "SUM(CASE WHEN (NO_INDEX_COL < 5000) THEN INT_COL ELSE 0 END) AS total_sum, "
+ "MAX(CASE WHEN (NO_INDEX_COL < 5000) THEN NO_INDEX_COL ELSE 0 END) AS total_max2 FROM MyTable";
+ }
+
+ @Test
+ public void testMixedAggregations() {
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] atris commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
atris commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r806536248
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -296,6 +296,19 @@ public void testMultipleAggregationsOnSameFilter() {
+ "MAX(CASE WHEN (INT_COL > 29990) THEN INT_COL ELSE 0 END) AS total_max, "
+ "SUM(CASE WHEN (NO_INDEX_COL < 5000) THEN INT_COL ELSE 0 END) AS total_sum, "
+ "MAX(CASE WHEN (NO_INDEX_COL < 5000) THEN NO_INDEX_COL ELSE 0 END) AS total_max2 FROM MyTable";
+ }
+
+ @Test
+ public void testMixedAggregations() {
Review comment:
Lets rename this to testMixedAggregationsOfSameType, since other tests in this class contain tests with mixed aggregations, just of different types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r803115591
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/request/context/QueryContext.java
##########
@@ -459,104 +457,87 @@ public QueryContext build() {
* Helper method to generate the aggregation functions for the query.
*/
private void generateAggregationFunctions(QueryContext queryContext) {
- List<AggregationFunction> aggregationFunctions = new ArrayList<>();
- List<Pair<AggregationFunction, FilterContext>> filteredAggregations = new ArrayList<>();
- Map<FunctionContext, Integer> aggregationFunctionIndexMap = new HashMap<>();
- Map<Pair<FunctionContext, FilterContext>, Integer> filterExpressionIndexMap = new HashMap<>();
+ List<Pair<AggregationFunction, FilterContext>> filteredAggregationFunctions = new ArrayList<>();
+ Map<Pair<FunctionContext, FilterContext>, Integer> filteredAggregationsIndexMap = new HashMap<>();
Review comment:
Want to make the naming consistent for readability:
- `Pair<AggregationFunction, FilterContext>` is named `filteredAggregationFunction`
- `Pair<FunctionContext, FilterContext>` is named `filteredAggregation`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1035555112
@atris Rebased the re-factor PR, and addressed the comments. Can you 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#issuecomment-1035562142
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8172](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f7e5b0e) into [master](https://codecov.io/gh/apache/pinot/commit/b404e4adf15c897321ef62b9e986af1ae7e946dc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b404e4a) will **decrease** coverage by `0.96%`.
> The diff coverage is `94.44%`.
[](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #8172 +/- ##
============================================
- Coverage 71.33% 70.37% -0.97%
Complexity 4303 4303
============================================
Files 1623 1623
Lines 84314 84304 -10
Branches 12640 12638 -2
============================================
- Hits 60146 59326 -820
- Misses 20047 20877 +830
+ Partials 4121 4101 -20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `28.86% <77.77%> (-0.02%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `67.88% <94.44%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.19% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...inot/core/query/reduce/PostAggregationHandler.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvUG9zdEFnZ3JlZ2F0aW9uSGFuZGxlci5qYXZh) | `91.76% <66.66%> (-0.10%)` | :arrow_down: |
| [...rg/apache/pinot/core/plan/AggregationPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0FnZ3JlZ2F0aW9uUGxhbk5vZGUuamF2YQ==) | `91.00% <100.00%> (+0.09%)` | :arrow_up: |
| [...pinot/core/query/request/context/QueryContext.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZXF1ZXN0L2NvbnRleHQvUXVlcnlDb250ZXh0LmphdmE=) | `97.96% <100.00%> (+0.38%)` | :arrow_up: |
| [...t/core/plan/StreamingInstanceResponsePlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ0luc3RhbmNlUmVzcG9uc2VQbGFuTm9kZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ore/operator/streaming/StreamingResponseUtils.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nUmVzcG9uc2VVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ager/realtime/PeerSchemeSplitSegmentCommitter.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUGVlclNjaGVtZVNwbGl0U2VnbWVudENvbW1pdHRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| [...he/pinot/core/plan/StreamingSelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1N0cmVhbWluZ1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [...ator/streaming/StreamingSelectionOnlyOperator.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seU9wZXJhdG9yLmphdmE=) | `0.00% <0.00%> (-87.81%)` | :arrow_down: |
| [...re/query/reduce/SelectionOnlyStreamingReducer.java](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvU2VsZWN0aW9uT25seVN0cmVhbWluZ1JlZHVjZXIuamF2YQ==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| ... and [94 more](https://codecov.io/gh/apache/pinot/pull/8172/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a899110...f7e5b0e](https://codecov.io/gh/apache/pinot/pull/8172?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #8172: Fix filtered aggregation when it is mixed with regular aggregation
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #8172:
URL: https://github.com/apache/pinot/pull/8172#discussion_r807115769
##########
File path: pinot-core/src/test/java/org/apache/pinot/queries/FilteredAggregationsTest.java
##########
@@ -296,6 +296,19 @@ public void testMultipleAggregationsOnSameFilter() {
+ "MAX(CASE WHEN (INT_COL > 29990) THEN INT_COL ELSE 0 END) AS total_max, "
+ "SUM(CASE WHEN (NO_INDEX_COL < 5000) THEN INT_COL ELSE 0 END) AS total_sum, "
+ "MAX(CASE WHEN (NO_INDEX_COL < 5000) THEN NO_INDEX_COL ELSE 0 END) AS total_max2 FROM MyTable";
+ }
+
+ @Test
+ public void testMixedAggregations() {
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org