Jackie-Jiang commented on code in PR #10092:
URL: https://github.com/apache/pinot/pull/10092#discussion_r1067550794
##########
pinot-common/src/main/java/org/apache/pinot/common/request/context/FilterContext.java:
##########
@@ -116,4 +116,8 @@ public String toString() {
throw new IllegalStateException();
}
}
+
+ public String getResultColumnName() {
Review Comment:
Suggest adding a util method in
`AggregationFunctionUtils.getResultColumnName(AggregationFunction
aggregationFunction, @Nullable FilterContext filter)` to directly return the
full result column name instead of adding it here. Also suggest making
`FILTER(WHERE` capitalized since they are preserved key word
##########
pinot-core/src/main/java/org/apache/pinot/core/data/table/TableResizer.java:
##########
@@ -407,13 +420,25 @@ public Comparable extract(Record record) {
*/
private class AggregationFunctionExtractor implements OrderByValueExtractor {
final int _index;
+ final boolean _isFilteredAgg;
final AggregationFunction _aggregationFunction;
AggregationFunctionExtractor(int aggregationFunctionIndex) {
_index = aggregationFunctionIndex + _numGroupByExpressions;
+ _isFilteredAgg = false;
_aggregationFunction = _aggregationFunctions[aggregationFunctionIndex];
}
+ AggregationFunctionExtractor(int aggregationFunctionIndex, boolean
isFilteredAgg) {
Review Comment:
Suggest changing the constructor to take both `aggregationFunctionIndex` and
`aggregationFunction` which is IMO easier to understand
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/AggregationResultsBlock.java:
##########
@@ -69,7 +84,14 @@ public DataSchema getDataSchema(QueryContext queryContext) {
ColumnDataType[] columnDataTypes = new ColumnDataType[numColumns];
for (int i = 0; i < numColumns; i++) {
AggregationFunction aggregationFunction = _aggregationFunctions[i];
- columnNames[i] = aggregationFunction.getColumnName();
+ String columnName = aggregationFunction.getResultColumnName();
Review Comment:
I checked and we are not using the column name returned from the server in
`AggregationDataTableReducer`. You may verify that by reverting the changes in
this class and see if all the new tests still pass. If that is the case, we can
also consider dropping the changes in this class to simplify the changes.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/query/FilteredGroupByOperator.java:
##########
@@ -60,8 +61,10 @@ public class FilteredGroupByOperator extends
BaseOperator<GroupByResultsBlock> {
private long _numEntriesScannedPostFilter;
private final DataSchema _dataSchema;
private final QueryContext _queryContext;
+ private final IdentityHashMap<AggregationFunction, Integer>
_resultHolderIndexMap;
Review Comment:
We probably can keep it as local variable. Don't see it being used in other
places
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/GroupByPlanNode.java:
##########
@@ -59,6 +59,7 @@ public Operator<GroupByResultsBlock> run() {
assert _queryContext.getGroupByExpressions() != null;
if (_queryContext.hasFilteredAggregations()) {
+ assert _queryContext.getFilteredAggregationFunctions() != null;
Review Comment:
For the context, this is never `null` even if there is no filter on
aggregation
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/blocks/results/ResultsBlockUtils.java:
##########
@@ -93,9 +99,17 @@ private static GroupByResultsBlock
buildEmptyGroupByQueryResults(QueryContext qu
columnDataTypes[index] = ColumnDataType.STRING;
index++;
}
- for (AggregationFunction aggregationFunction : aggregationFunctions) {
+ for (int i = 0; i < aggregationFunctions.length; i++) {
// NOTE: Use AggregationFunction.getResultColumnName() for SQL format
response
- columnNames[index] = aggregationFunction.getResultColumnName();
+ AggregationFunction aggregationFunction = aggregationFunctions[i];
+ String columnName = aggregationFunction.getResultColumnName();
+ if (filteredAggregationFunctions != null) {
Review Comment:
`filteredAggregationFunctions` should never be `null`. We can actually
remove `aggregationFunctions` and always get `aggregationFunction` from
`filteredAggregationFunctions`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
