Skip to content

Commit 7aba9ff

Browse files
xiangfu0claude
andcommitted
Fix trim stats propagation and address review findings
1. Propagate _numResizes/_resizeTimeNs in mergePartitionTable() so isTrimmed() reflects trimming from absorbed tables. Also add absorbResizeStats() for the merge() path used in publishLocalPartitions(). Without this, unsafe-trimmed results could be reported as exact. 2. Add regression test testMergePartitionTablePropagatesResizeStats() that verifies groupsTrimmed, numResizes, and resizeTimeMs survive absorbed-table merges via both mergePartitionTable() and merge()+absorbResizeStats(). 3. Fix Javadoc on mergeResults(): "selection result blocks" -> "group-by result blocks" with accurate description of partition tree-reduction. 4. Fix timeout/error strings: "group-by order-by" -> "group-by", and clamp timeout to non-negative in error message. 5. Add Javadoc to mergePartitionTable() documenting disjoint-keys precondition. 6. Fix integration test: remove stale groupByAlgorithm query option, use only numGroupByPartitions to control partitioned combine. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 39c2463 commit 7aba9ff

5 files changed

Lines changed: 80 additions & 12 deletions

File tree

pinot-core/src/main/java/org/apache/pinot/core/data/table/IndexedTable.java

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,10 +287,20 @@ public Iterator<Record> iterator() {
287287
}
288288

289289

290+
/**
291+
* Merges records from a disjoint partition table into this table using {@code putAll}.
292+
* <p><b>Precondition:</b> the source table must have no overlapping keys with this table
293+
* (i.e. they come from disjoint hash partitions). If keys overlap, {@code putAll} will
294+
* silently overwrite existing entries instead of aggregating them. Use {@link #merge(Table)}
295+
* for tables that may share keys.
296+
*/
290297
public void mergePartitionTable(Table table) {
291298
_topRecords = null;
292299
if (table instanceof IndexedTable) {
293-
_lookupMap.putAll(((IndexedTable) table)._lookupMap);
300+
IndexedTable other = (IndexedTable) table;
301+
_lookupMap.putAll(other._lookupMap);
302+
_numResizes += other._numResizes;
303+
_resizeTimeNs += other._resizeTimeNs;
294304
} else {
295305
Iterator<Record> iterator = table.iterator();
296306
while (iterator.hasNext()) {
@@ -304,6 +314,15 @@ public void mergePartitionTable(Table table) {
304314
}
305315
}
306316

317+
/**
318+
* Absorbs trim/resize statistics from another table that was merged into this one.
319+
* Call after {@link #merge(Table)} to preserve the absorbed table's trim history.
320+
*/
321+
public void absorbResizeStats(IndexedTable other) {
322+
_numResizes += other._numResizes;
323+
_resizeTimeNs += other._resizeTimeNs;
324+
}
325+
307326
public int getNumResizes() {
308327
return _numResizes;
309328
}

pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByCombineOperator.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,14 +219,14 @@ public BaseResultsBlock mergeResults()
219219
}
220220

221221
protected ExceptionResultsBlock getTimeoutResultsBlock(long timeoutMs) {
222-
String userError = "Timed out while combining group-by order-by results after " + timeoutMs + "ms";
222+
String userError = "Timed out while combining group-by results after " + Math.max(0, timeoutMs) + "ms";
223223
String logMsg = userError + ", queryContext = " + _queryContext;
224224
LOGGER.error(logMsg);
225225
return new ExceptionResultsBlock(new QueryErrorMessage(QueryErrorCode.EXECUTION_TIMEOUT, userError, logMsg));
226226
}
227227

228228
protected ExceptionResultsBlock getExceptionResultsBlock(Throwable ex) {
229-
String userError = "Caught exception while processing group-by order-by query";
229+
String userError = "Caught exception while processing group-by query";
230230
String devError = userError + ": " + ex.getMessage();
231231
QueryErrorMessage errMsg;
232232
if (ex instanceof QueryException) {

pinot-core/src/main/java/org/apache/pinot/core/operator/combine/PartitionedGroupByCombineOperator.java

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,14 +110,10 @@ protected void processSegments() {
110110
/**
111111
* {@inheritDoc}
112112
*
113-
* <p>Combines intermediate selection result blocks from underlying operators and returns a merged one.
113+
* <p>Combines intermediate group-by result blocks from underlying operators and returns a merged one.
114114
* <ul>
115-
* <li>
116-
* Merges multiple intermediate selection result blocks as a merged one.
117-
* </li>
118-
* <li>
119-
* Set all exceptions encountered during execution into the merged result block
120-
* </li>
115+
* <li>Merges partition tables into a single table via parallel tree-reduction.</li>
116+
* <li>Sets all exceptions encountered during execution into the merged result block.</li>
121117
* </ul>
122118
*/
123119
@Override
@@ -215,9 +211,11 @@ private void publishLocalPartitions(IndexedTable[] localIndexedTables) {
215211
_indexedTables[partitionId] = localIndexedTable;
216212
} else if (localIndexedTable.size() > indexedTable.size()) {
217213
localIndexedTable.merge(indexedTable);
214+
localIndexedTable.absorbResizeStats(indexedTable);
218215
_indexedTables[partitionId] = localIndexedTable;
219216
} else {
220217
indexedTable.merge(localIndexedTable);
218+
indexedTable.absorbResizeStats(localIndexedTable);
221219
}
222220
}
223221
}

pinot-core/src/test/java/org/apache/pinot/core/data/table/IndexedTableTest.java

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,4 +337,56 @@ public void testMergePartitionTableInvalidatesFinishedTopRecords() {
337337
executorService.shutdownNow();
338338
}
339339
}
340+
341+
@Test
342+
public void testMergePartitionTablePropagatesResizeStats() {
343+
QueryContext queryContext = QueryContextConverterUtils.getQueryContext(
344+
"SELECT SUM(m1) FROM testTable GROUP BY d1 ORDER BY SUM(m1) DESC");
345+
DataSchema dataSchema = new DataSchema(new String[]{"d1", "sum(m1)"},
346+
new ColumnDataType[]{ColumnDataType.STRING, ColumnDataType.DOUBLE});
347+
348+
ExecutorService executorService = Executors.newCachedThreadPool();
349+
try {
350+
// Create a table that will be trimmed (trimThreshold=5, trimSize=3)
351+
IndexedTable trimmedTable =
352+
new SimpleIndexedTable(dataSchema, false, queryContext, 3, 3, 5, INITIAL_CAPACITY, executorService);
353+
for (int i = 0; i < 6; i++) {
354+
trimmedTable.upsert(new Key(new Object[]{"k" + i}), getRecord(new Object[]{"k" + i, (double) i}));
355+
}
356+
Assert.assertTrue(trimmedTable.isTrimmed(), "trimmedTable should be trimmed after exceeding threshold");
357+
int trimmedResizes = trimmedTable.getNumResizes();
358+
Assert.assertTrue(trimmedResizes > 0);
359+
360+
// Create a non-trimmed table
361+
IndexedTable survivingTable =
362+
new SimpleIndexedTable(dataSchema, false, queryContext, 100, 100, Integer.MAX_VALUE, INITIAL_CAPACITY,
363+
executorService);
364+
survivingTable.upsert(new Key(new Object[]{"x"}), getRecord(new Object[]{"x", 99d}));
365+
Assert.assertFalse(survivingTable.isTrimmed());
366+
Assert.assertEquals(survivingTable.getNumResizes(), 0);
367+
368+
// mergePartitionTable should propagate resize stats
369+
survivingTable.mergePartitionTable(trimmedTable);
370+
Assert.assertEquals(survivingTable.getNumResizes(), trimmedResizes);
371+
Assert.assertTrue(survivingTable.isTrimmed(),
372+
"Surviving table should report trimmed after absorbing a trimmed table");
373+
374+
// absorbResizeStats should also work for merge() path
375+
IndexedTable tableA =
376+
new SimpleIndexedTable(dataSchema, false, queryContext, 100, 100, Integer.MAX_VALUE, INITIAL_CAPACITY,
377+
executorService);
378+
IndexedTable tableB =
379+
new SimpleIndexedTable(dataSchema, false, queryContext, 3, 3, 5, INITIAL_CAPACITY, executorService);
380+
for (int i = 0; i < 6; i++) {
381+
tableB.upsert(new Key(new Object[]{"b" + i}), getRecord(new Object[]{"b" + i, (double) i}));
382+
}
383+
Assert.assertTrue(tableB.isTrimmed());
384+
tableA.merge(tableB);
385+
tableA.absorbResizeStats(tableB);
386+
Assert.assertTrue(tableA.isTrimmed(),
387+
"tableA should report trimmed after merge + absorbResizeStats from trimmed tableB");
388+
} finally {
389+
executorService.shutdownNow();
390+
}
391+
}
340392
}

pinot-core/src/test/java/org/apache/pinot/queries/InterSegmentPartitionedGroupBySingleValueQueriesTest.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,7 @@
3030
public class InterSegmentPartitionedGroupBySingleValueQueriesTest
3131
extends InterSegmentGroupBySingleValueQueriesTest {
3232
private static final InstancePlanMakerImplV2 TRIM_ENABLED_PLAN_MAKER = new InstancePlanMakerImplV2();
33-
private static final Map<String, String> QUERY_OPTIONS =
34-
Map.of("groupByAlgorithm", "partitioned", "numGroupByPartitions", "8");
33+
private static final Map<String, String> QUERY_OPTIONS = Map.of("numGroupByPartitions", "8");
3534

3635
static {
3736
TRIM_ENABLED_PLAN_MAKER.setMinSegmentGroupTrimSize(1);

0 commit comments

Comments
 (0)