Copilot commented on code in PR #17815:
URL: https://github.com/apache/pinot/pull/17815#discussion_r2885882475
##########
pinot-broker/src/test/java/org/apache/pinot/broker/requesthandler/EmptyResponseUtilsTest.java:
##########
@@ -140,6 +140,37 @@ public void testBuildEmptyResultTableWithAliases() {
assertTrue(resultTable.getRows().isEmpty());
}
+ @Test
+ public void testBuildEmptyResultTableWithPostAggregation() {
+ // Aggregation with post-aggregation expression and aliases
+ QueryContext queryContext = QueryContextConverterUtils.getQueryContext(
+ "SELECT SUM(a) * 1.0 / COUNT(*) AS rate, SUM(a) AS total FROM
testTable WHERE foo = 'bar'");
+ ResultTable resultTable =
EmptyResponseUtils.buildEmptyResultTable(queryContext);
+ DataSchema dataSchema = resultTable.getDataSchema();
+ assertEquals(dataSchema.getColumnNames(), new String[]{"rate", "total"});
+ assertEquals(dataSchema.size(), 2);
+ List<Object[]> rows = resultTable.getRows();
+ assertEquals(rows.size(), 1);
+
+ // Aggregation with post-aggregation expression without alias
+ queryContext = QueryContextConverterUtils.getQueryContext(
+ "SELECT SUM(a) + MAX(b) FROM testTable WHERE foo = 'bar'");
+ resultTable = EmptyResponseUtils.buildEmptyResultTable(queryContext);
+ dataSchema = resultTable.getDataSchema();
+ assertEquals(dataSchema.size(), 1);
+ rows = resultTable.getRows();
+ assertEquals(rows.size(), 1);
Review Comment:
The post-aggregation empty-response tests only assert column names/size, but
the change also relies on PostAggregationHandler to produce the correct output
column data types (e.g., `rate` should be DOUBLE). Adding assertions for
`dataSchema.getColumnDataTypes()` here would better lock in the intended schema
behavior and prevent silent regressions.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/EmptyResponseUtils.java:
##########
@@ -80,59 +81,84 @@ private static ResultTable
buildEmptySelectionResultTable(QueryContext queryCont
private static ResultTable buildEmptyAggregationResultTable(QueryContext
queryContext) {
List<Pair<AggregationFunction, FilterContext>>
filteredAggregationFunctions =
queryContext.getFilteredAggregationFunctions();
- List<String> aliases = queryContext.getAliasList();
assert filteredAggregationFunctions != null;
int numAggregations = filteredAggregationFunctions.size();
- String[] columnNames = new String[numAggregations];
- ColumnDataType[] columnDataTypes = new ColumnDataType[numAggregations];
- Object[] row = new Object[numAggregations];
+
+ // Build pre-post-aggregation DataSchema from individual aggregation
functions
+ String[] preAggColumnNames = new String[numAggregations];
+ ColumnDataType[] preAggColumnDataTypes = new
ColumnDataType[numAggregations];
+ Object[] rawRow = new Object[numAggregations];
for (int i = 0; i < numAggregations; i++) {
Pair<AggregationFunction, FilterContext> pair =
filteredAggregationFunctions.get(i);
AggregationFunction aggregationFunction = pair.getLeft();
- if (aliases.size() == numAggregations && aliases.get(i) != null) {
- columnNames[i] = aliases.get(i);
- } else {
- columnNames[i] =
AggregationFunctionUtils.getResultColumnName(aggregationFunction,
pair.getRight());
- }
- columnDataTypes[i] = aggregationFunction.getFinalResultColumnType();
+ preAggColumnNames[i] =
AggregationFunctionUtils.getResultColumnName(aggregationFunction,
pair.getRight());
+ preAggColumnDataTypes[i] =
aggregationFunction.getFinalResultColumnType();
Object finalResult = aggregationFunction.extractFinalResult(
aggregationFunction.extractAggregationResult(aggregationFunction.createAggregationResultHolder()));
- row[i] = finalResult != null ? columnDataTypes[i].convert(finalResult) :
null;
+ rawRow[i] = finalResult != null ?
preAggColumnDataTypes[i].convert(finalResult) : null;
}
- return new ResultTable(new DataSchema(columnNames, columnDataTypes),
List.<Object[]>of(row));
+ DataSchema prePostAggDataSchema = new DataSchema(preAggColumnNames,
preAggColumnDataTypes);
+
+ // Use PostAggregationHandler to evaluate post-aggregation expressions and
reorder columns
+ PostAggregationHandler postAggregationHandler = new
PostAggregationHandler(queryContext, prePostAggDataSchema);
+ DataSchema resultDataSchema = postAggregationHandler.getResultDataSchema();
+ Object[] resultRow = postAggregationHandler.getResult(rawRow);
+
+ applyAliases(queryContext, resultDataSchema);
+ return new ResultTable(resultDataSchema, List.<Object[]>of(resultRow));
}
private static ResultTable buildEmptyGroupByResultTable(QueryContext
queryContext) {
List<Pair<AggregationFunction, FilterContext>>
filteredAggregationFunctions =
queryContext.getFilteredAggregationFunctions();
- List<String> aliases = queryContext.getAliasList();
List<ExpressionContext> groupByExpressions =
queryContext.getGroupByExpressions();
assert filteredAggregationFunctions != null && groupByExpressions != null;
- int numColumns = groupByExpressions.size() +
filteredAggregationFunctions.size();
- String[] columnNames = new String[numColumns];
- ColumnDataType[] columnDataTypes = new ColumnDataType[numColumns];
- int index = 0;
- for (ExpressionContext groupByExpression : groupByExpressions) {
- if (aliases.size() == numColumns && aliases.get(index) != null) {
- columnNames[index] = aliases.get(index);
- } else {
- columnNames[index] = groupByExpression.toString();
- }
+ int numGroupByExpressions = groupByExpressions.size();
+ int numAggregations = filteredAggregationFunctions.size();
+ int numColumns = numGroupByExpressions + numAggregations;
+
+ // Build pre-post-aggregation DataSchema with group-by columns followed by
aggregation columns
+ String[] preAggColumnNames = new String[numColumns];
+ ColumnDataType[] preAggColumnDataTypes = new ColumnDataType[numColumns];
+ for (int i = 0; i < numGroupByExpressions; i++) {
+ preAggColumnNames[i] = groupByExpressions.get(i).toString();
// Use STRING column data type as default for group-by expressions
- columnDataTypes[index] = ColumnDataType.STRING;
- index++;
+ preAggColumnDataTypes[i] = ColumnDataType.STRING;
}
- for (Pair<AggregationFunction, FilterContext> pair :
filteredAggregationFunctions) {
+ for (int i = 0; i < numAggregations; i++) {
+ Pair<AggregationFunction, FilterContext> pair =
filteredAggregationFunctions.get(i);
AggregationFunction aggregationFunction = pair.getLeft();
- if (aliases.size() == numColumns && aliases.get(index) != null) {
- columnNames[index] = aliases.get(index);
- } else {
- columnNames[index] =
AggregationFunctionUtils.getResultColumnName(aggregationFunction,
pair.getRight());
+ preAggColumnNames[numGroupByExpressions + i] =
+ AggregationFunctionUtils.getResultColumnName(aggregationFunction,
pair.getRight());
+ preAggColumnDataTypes[numGroupByExpressions + i] =
aggregationFunction.getFinalResultColumnType();
+ }
+ DataSchema prePostAggDataSchema = new DataSchema(preAggColumnNames,
preAggColumnDataTypes);
+
+ // Use PostAggregationHandler to evaluate post-aggregation expressions and
reorder columns
+ PostAggregationHandler postAggregationHandler = new
PostAggregationHandler(queryContext, prePostAggDataSchema);
+ DataSchema resultDataSchema = postAggregationHandler.getResultDataSchema();
+
+ applyAliases(queryContext, resultDataSchema);
+ return new ResultTable(resultDataSchema, List.of());
+ }
+
+ private static void applyAliases(QueryContext queryContext, DataSchema
dataSchema) {
+ List<String> aliasList = queryContext.getAliasList();
+ if (aliasList.isEmpty()) {
+ return;
+ }
+ String[] columnNames = dataSchema.getColumnNames();
+ List<ExpressionContext> selectExpressions =
queryContext.getSelectExpressions();
+ int numSelectExpressions = selectExpressions.size();
+ if (columnNames.length != numSelectExpressions) {
+ return;
+ }
+ for (int i = 0; i < numSelectExpressions; i++) {
Review Comment:
`applyAliases()` checks `aliasList.isEmpty()`, but
`QueryContextConverterUtils` populates `_aliasList` with `null` placeholders
(so the list is typically non-empty even when there are no aliases). This makes
the early-return ineffective and the method will always scan/attempt to apply
aliases. Consider short-circuiting by checking for at least one non-null alias
(or documenting that aliasList may contain null placeholders).
```suggestion
if (aliasList == null || aliasList.isEmpty()) {
return;
}
String[] columnNames = dataSchema.getColumnNames();
List<ExpressionContext> selectExpressions =
queryContext.getSelectExpressions();
int numSelectExpressions = selectExpressions.size();
if (columnNames.length != numSelectExpressions || aliasList.size() !=
numSelectExpressions) {
return;
}
boolean hasAlias = false;
for (int i = 0; i < numSelectExpressions; i++) {
if (aliasList.get(i) != null) {
hasAlias = true;
break;
}
}
if (!hasAlias) {
return;
}
for (int i = 0; i < numSelectExpressions; i++) {
```
--
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]
