Copilot commented on code in PR #18842:
URL: https://github.com/apache/pinot/pull/18842#discussion_r3531450092
##########
pinot-core/src/test/java/org/apache/pinot/core/query/reduce/MergeDataTablesOnlyTest.java:
##########
@@ -118,6 +118,50 @@ public void testGroupByRoundTrip() {
assertRoundTrip(query, serverTables);
}
+ /**
+ * Regression for the merge-only-reducer GROUP BY + DISTINCTCOUNT defect:
prior to the fix,
+ * {@code mergeDataTablesOnly} called {@code getIndexedTable} which
unconditionally finished the
+ * IndexedTable with {@code storeFinalResult=true} — finalizing each row's
aggregate (Set → Integer).
+ * The subsequent {@code buildIntermediateDataTable} then read {@code
_storedColumnDataTypes} (still
+ * OBJECT), took the OBJECT branch, and handed the now-Integer value into
+ * {@code
BaseDistinctAggregateAggregationFunction.serializeIntermediateResult(Set)} →
+ * {@link ClassCastException}. This test fails without the fix and passes
with it.
+ *
+ * <p>Builds fresh per-server DataTables for the {@code reduce(...)}
baseline and the
+ * {@code merge(...)} call separately. The regular reduce path mutates the
input DataTable's
+ * schema's column-data-types array in place via {@code
IndexedTable.finish(_, true)} — that's
+ * pre-existing OSS behavior and is fine in production where no caller
re-reads the input after
+ * a reduce. Reusing the same DataTable list across both calls would falsely
surface that
+ * mutation as an apparent merge-only bug.
+ */
+ @Test
+ public void testGroupByDistinctCountObjectRoundTrip()
+ throws IOException {
+ String query = "SELECT col1, DISTINCTCOUNT(col2) FROM testTable GROUP BY
col1";
+ BrokerRequest brokerRequest =
CalciteSqlCompiler.compileToBrokerRequest(query);
+ BrokerResponseNative baseline = reduce(brokerRequest,
toMap(buildDistinctCountServerTables(query)));
+ DataTable merged = merge(brokerRequest,
toMap(buildDistinctCountServerTables(query)));
+ assertNotNull(merged, "merge produced null");
+ BrokerResponseNative viaMerge = reduce(brokerRequest,
singletonMap(merged));
+ assertResultTablesEquivalent(baseline.getResultTable(),
viaMerge.getResultTable());
+ }
+
+ /** Two per-server intermediate DataTables for {@code GROUP BY col1,
DISTINCTCOUNT(col2)} with one
+ * overlapping group across servers so the OBJECT-path per-group Set merge
actually runs. */
Review Comment:
This Javadoc says "GROUP BY col1, DISTINCTCOUNT(col2)", which reads like
`DISTINCTCOUNT(col2)` is part of the GROUP BY clause. It’s actually a `GROUP BY
col1` query selecting `DISTINCTCOUNT(col2)`, so rephrasing avoids confusion for
future readers.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/GroupByDataTableReducer.java:
##########
@@ -148,6 +148,13 @@ public DataTable mergeDataTablesOnly(String tableName,
DataSchema dataSchema,
Collection<DataTable> dataTables = dataTableMap.values();
// Reuse the regular reduce's merge: builds the IndexedTable of group
keys + intermediate agg state.
IndexedTable indexedTable = getIndexedTable(dataSchema, dataTables,
reducerContext);
Review Comment:
`mergeDataTablesOnly()` rejects `isServerReturnFinalResult()`, but it does
not reject `isServerReturnFinalResultKeyUnpartitioned()`. That option also
makes servers return final (not intermediate) aggregate values, so merge-only
cannot safely produce a re-mergeable intermediate DataTable and should fail
fast the same way.
--
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]
