xiangfu0 commented on code in PR #19601:
URL: https://github.com/apache/pinot/pull/19601#discussion_r4065233611
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -446,16 +446,27 @@ private int[] generateGroupByKeys(List<Object[]> rows) {
}
private int[] generateGroupByKeys(DataBlock dataBlock) {
- Object[] keys;
- if (_groupKeyIds.length == 1) {
- keys = DataBlockExtractUtils.extractKey(dataBlock, _groupKeyIds[0]);
- } else {
- keys = DataBlockExtractUtils.extractKeys(dataBlock, _groupKeyIds);
- }
- int numRows = keys.length;
+ int numRows = dataBlock.getNumberOfRows();
int[] intKeys = new int[numRows];
- for (int i = 0; i < numRows; i++) {
- intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ int numKeys = _groupKeyIds.length;
+ if (numKeys == 1) {
+ Object[] keys = DataBlockExtractUtils.extractKey(dataBlock,
_groupKeyIds[0]);
+ for (int i = 0; i < numRows; i++) {
+ intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ }
+ } else {
+ Object[][] columns = new Object[numKeys][];
+ for (int i = 0; i < numKeys; i++) {
+ columns[i] = DataBlockExtractUtils.extractKey(dataBlock,
_groupKeyIds[i]);
+ }
+ // Multi-column generators retain dictionary IDs, not this array, just
as in the row-heap path.
+ Object[] key = new Object[numKeys];
Review Comment:
Moved the ownership guarantee to GroupIdGenerator#getGroupId: the
multi-column input is scratch storage, may be mutated immediately after return,
and must not be retained. Removed the caller-only comment.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -446,16 +446,27 @@ private int[] generateGroupByKeys(List<Object[]> rows) {
}
private int[] generateGroupByKeys(DataBlock dataBlock) {
- Object[] keys;
- if (_groupKeyIds.length == 1) {
- keys = DataBlockExtractUtils.extractKey(dataBlock, _groupKeyIds[0]);
- } else {
- keys = DataBlockExtractUtils.extractKeys(dataBlock, _groupKeyIds);
- }
- int numRows = keys.length;
+ int numRows = dataBlock.getNumberOfRows();
int[] intKeys = new int[numRows];
- for (int i = 0; i < numRows; i++) {
- intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ int numKeys = _groupKeyIds.length;
+ if (numKeys == 1) {
+ Object[] keys = DataBlockExtractUtils.extractKey(dataBlock,
_groupKeyIds[0]);
+ for (int i = 0; i < numRows; i++) {
+ intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ }
+ } else {
+ Object[][] columns = new Object[numKeys][];
Review Comment:
Rewrote the description to lead with removal of the row-oriented key matrix
and its per-row arrays, in favor of column arrays plus one scratch key. Removed
the unsupported escape-analysis attribution. Exact byte savings depend on
reference width/object layout; the recorded executor measurement is explicitly
a combined-change result.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -446,16 +446,27 @@ private int[] generateGroupByKeys(List<Object[]> rows) {
}
private int[] generateGroupByKeys(DataBlock dataBlock) {
- Object[] keys;
- if (_groupKeyIds.length == 1) {
- keys = DataBlockExtractUtils.extractKey(dataBlock, _groupKeyIds[0]);
- } else {
- keys = DataBlockExtractUtils.extractKeys(dataBlock, _groupKeyIds);
- }
- int numRows = keys.length;
+ int numRows = dataBlock.getNumberOfRows();
int[] intKeys = new int[numRows];
- for (int i = 0; i < numRows; i++) {
- intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ int numKeys = _groupKeyIds.length;
+ if (numKeys == 1) {
+ Object[] keys = DataBlockExtractUtils.extractKey(dataBlock,
_groupKeyIds[0]);
+ for (int i = 0; i < numRows; i++) {
+ intKeys[i] = _groupIdGenerator.getGroupId(keys[i]);
+ }
+ } else {
+ Object[][] columns = new Object[numKeys][];
+ for (int i = 0; i < numKeys; i++) {
+ columns[i] = DataBlockExtractUtils.extractKey(dataBlock,
_groupKeyIds[i]);
+ }
+ // Multi-column generators retain dictionary IDs, not this array, just
as in the row-heap path.
+ Object[] key = new Object[numKeys];
+ for (int rowId = 0; rowId < numRows; rowId++) {
+ for (int i = 0; i < numKeys; i++) {
+ key[i] = columns[i][rowId];
+ }
+ intKeys[rowId] = _groupIdGenerator.getGroupId(key);
Review Comment:
Included the filtered serialized path using the existing bitmap-aware
extractKey helper for each column and one scratch array. Added two-/three-key
filtered tests covering sparse matches, null keys, multiple blocks,
empty/all-unmatched blocks, and both empty-group policies.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultistageGroupByExecutor.java:
##########
@@ -372,7 +372,7 @@ private void processMerge(MseBlock.Data block) {
int[] groupByKeys = generateGroupByKeys(block);
int numRows = groupByKeys.length;
int numFunctions = _aggFunctions.length;
- Object[][] intermediateResults = new Object[numFunctions][numRows];
+ Object[][] intermediateResults = new Object[numFunctions][];
Review Comment:
Clarified this in the description: every outer entry receives a non-null
array from getIntermediateResults before use. The overwritten allocations
provided no downstream length validation, so this removes dead allocations
without changing that contract.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/utils/IntToIdMap.java:
##########
@@ -36,9 +36,10 @@ public IntToIdMap() {
@Override
public int put(int value) {
int numValues = _valueToIdMap.size();
- int id = _valueToIdMap.computeIfAbsent(value, k -> numValues);
- if (id == numValues) {
+ int id = _valueToIdMap.putIfAbsent(value, numValues);
+ if (id == INVALID_KEY) {
Review Comment:
Agreed: assigned IDs are below the next ID, so the old absence check was
correct. Removed the latent-bug/sentinel-fix claims. Included ObjectToIdMap in
the putIfAbsent change and covered its STRING/BYTES/BIG_DECIMAL factory paths
in the shared map test.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/utils/DoubleToIdMap.java:
##########
@@ -36,9 +36,10 @@ public DoubleToIdMap() {
@Override
public int put(double value) {
int numValues = _valueToIdMap.size();
- int id = _valueToIdMap.computeIfAbsent(value, k -> numValues);
- if (id == numValues) {
+ int id = _valueToIdMap.putIfAbsent(value, numValues);
Review Comment:
Updated the description to distinguish the concrete open-hash-map
single-lookup override from the interface default. The measurements show no
dictionary-allocation improvement, and the small timing differences do not
establish a latency change. I removed the unsupported performance/correctness
attribution and kept the original numbers with their exact historical revisions.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/TypeUtils.java:
##########
@@ -39,13 +39,13 @@ private TypeUtils() {
public static Object convert(Object value, ColumnDataType storedType) {
switch (storedType) {
case INT:
- return ((Number) value).intValue();
+ return value instanceof Integer ? value : ((Number) value).intValue();
Review Comment:
Broadened the title and description to cover MSE grouping and result
conversion. One nuance: LeafOperator already skips columns with matching stored
types, so the description avoids claiming every leaf query benefits. I am
deferring precomputed conversion dispatch: convertRow has only target types,
while aggregate/function result representations can still need conversion.
LeafOperator already has schema-based skipping where both schemas are available.
Updated in `1a76016e09`; the combined current-source run passed all 190
selected tests.
--
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]
