Jackie-Jiang commented on code in PR #19357:
URL: https://github.com/apache/pinot/pull/19357#discussion_r3855944156
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/ParentExprMinMaxAggregationFunction.java:
##########
@@ -111,25 +116,26 @@ public GroupByResultHolder createGroupByResultHolder(int
initialCapacity, int ma
public void aggregate(int length, AggregationResultHolder
aggregationResultHolder,
Map<ExpressionContext, BlockValSet> blockValSetMap) {
- ExprMinMaxObject exprMinMaxObject = aggregationResultHolder.getResult();
+ initializeWithNewDataBlocks(blockValSetMap);
- if (exprMinMaxObject == null) {
- initializeWithNewDataBlocks(blockValSetMap);
- exprMinMaxObject = new ExprMinMaxObject(_measuringColumnSchema.get(),
_projectionColumnSchema.get());
- }
+ ExprMinMaxObject existing = aggregationResultHolder.getResult();
+ ExprMinMaxObject exprMinMaxObject = existing != null ? existing
+ : new ExprMinMaxObject(_measuringColumnSchema.get(),
_projectionColumnSchema.get());
List<Integer> rowIds = new ArrayList<>();
- for (int i = 0; i < length; i++) {
- int compareResult =
exprMinMaxObject.compareAndSetKey(_exprMinMaxWrapperMeasuringColumnSets.get(),
i, _isMax);
- if (compareResult == 0) {
- // same key, add the rowId to the list
- rowIds.add(i);
- } else if (compareResult > 0) {
- // new key is set, clear the list and add the new rowId
- rowIds.clear();
- rowIds.add(i);
+ forEachNotNullMeasuring(length, blockValSetMap, (from, to) -> {
+ for (int i = from; i < to; i++) {
+ int compareResult =
exprMinMaxObject.compareAndSetKey(_exprMinMaxWrapperMeasuringColumnSets.get(),
i, _isMax);
+ if (compareResult == 0) {
+ // same key, add the rowId to the list
+ rowIds.add(i);
+ } else if (compareResult > 0) {
+ // new key is set, clear the list and add the new rowId
+ rowIds.clear();
+ rowIds.add(i);
+ }
}
- }
+ });
// for all the rows that are associated with the extremum key, add the
projection columns
for (Integer rowId : rowIds) {
Review Comment:
Fixed in the latest push, exactly as you describe: the block records whether
it replaced the key, and the first surviving row is published through
`setToNewVal` so the earlier key's values are discarded, with the rows tying it
appended after.
I kept the batching rather than inlining it to match `updateGroupByResult`,
since collecting the winners first means a row beaten within its own block
never reads the projection columns at all — inlining would call `setToNewVal`
once per new extremum, so n times on a descending column.
One thing worth recording: this is not a symptom the rebind fix introduced.
On master the test fails too, just earlier — at the key assertion rather than
the row count, because the second block re-reads the first block's values so
the key never advances to `3`. Master gets the extremum, the row count and the
projection value all wrong for a multi-block `EXPR_MIN`; the rebind fix
corrected the key and moved the failure onto the second defect. I have reframed
the PR description accordingly: one multi-block correctness bug with two
causes, neither of which helps on its own.
Thanks for the 4 MB log-cap detail — that explains why I read the run as
pending rather than failing.
Also added since your review: a tie-after-replacement case, and three
group-by tests. The group-by path needed no change, but it had no coverage at
all despite this PR adding null gating to both `aggregateGroupBySV` and
`aggregateGroupByMV`.
--
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]
