xiangfu0 commented on code in PR #19834:
URL: https://github.com/apache/pinot/pull/19834#discussion_r4278279149
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/MultiKeysGroupIdGenerator.java:
##########
@@ -73,7 +90,7 @@ public int getGroupId(Object key) {
keyIds[i] = keyId;
}
}
- return _groupIdMap.getInt(new FixedIntArray(keyIds));
+ return _groupIdMap.getInt(currentKey);
Review Comment:
Correctness looks good: `getInt` is a pure lookup, so reusing `currentKey`
here is safe and the map never aliases the field on this path. Nit: a short
comment mirroring the new-group branch (e.g. "lookup only, no ownership
transfer") would make the asymmetry with the insert path obvious to future
readers.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/MultiKeysGroupIdGenerator.java:
##########
@@ -47,19 +56,27 @@ public MultiKeysGroupIdGenerator(ColumnDataType[] keyTypes,
int numKeyColumns,
_keyToIdMaps[i] = ValueToIdMapFactory.get(keyTypes[i].toDataType());
}
_numGroupsLimit = numGroupsLimit;
+ _currentKey = new FixedIntArray(new int[numKeyColumns]);
}
@Override
public int getGroupId(Object key) {
Object[] keyValues = (Object[]) key;
- int numKeyColumns = keyValues.length;
- int[] keyIds = new int[numKeyColumns];
- if (_groupIdMap.size() < _numGroupsLimit) {
+ int numKeyColumns = _keyToIdMaps.length;
Review Comment:
Minor: this switches the loop bound from `keyValues.length` to
`_keyToIdMaps.length`. It's a nice robustness improvement (the row array no
longer has to be exactly `numKeyColumns` long), and the caller always sizes it
exactly so behavior is unchanged in practice. Just flagging it as an
intentional change rather than a pure refactor.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/MultiKeysGroupIdGenerator.java:
##########
@@ -47,19 +56,27 @@ public MultiKeysGroupIdGenerator(ColumnDataType[] keyTypes,
int numKeyColumns,
_keyToIdMaps[i] = ValueToIdMapFactory.get(keyTypes[i].toDataType());
}
_numGroupsLimit = numGroupsLimit;
+ _currentKey = new FixedIntArray(new int[numKeyColumns]);
}
@Override
public int getGroupId(Object key) {
Object[] keyValues = (Object[]) key;
- int numKeyColumns = keyValues.length;
- int[] keyIds = new int[numKeyColumns];
- if (_groupIdMap.size() < _numGroupsLimit) {
+ int numKeyColumns = _keyToIdMaps.length;
Review Comment:
Minor: this switches the loop bound from `keyValues.length` to
`_keyToIdMaps.length`. It's a nice robustness improvement (the row array no
longer has to be exactly `numKeyColumns` long), and the caller always sizes it
exactly so behavior is unchanged in practice. Just flagging it as an
intentional change rather than a pure refactor.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/groupby/MultiKeysGroupIdGenerator.java:
##########
@@ -73,7 +90,7 @@ public int getGroupId(Object key) {
keyIds[i] = keyId;
}
}
- return _groupIdMap.getInt(new FixedIntArray(keyIds));
+ return _groupIdMap.getInt(currentKey);
Review Comment:
Correctness looks good: `getInt` is a pure lookup, so reusing `currentKey`
here is safe and the map never aliases the field on this path. Nit: a short
comment mirroring the new-group branch (e.g. "lookup only, no ownership
transfer") would make the asymmetry with the insert path obvious to future
readers.
--
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]
