yashmayya commented on code in PR #19390:
URL: https://github.com/apache/pinot/pull/19390#discussion_r3909650681
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java:
##########
@@ -132,6 +166,15 @@ public
DictionaryBasedGroupKeyGenerator(BaseProjectOperator<?> projectOperator,
}
_isSingleValueColumn[i] = columnContext.isSingleValue();
}
+ if (anyNullDictId) {
+ _nullDictIds = nullDictIds;
+ _nullReplacedSingleValueDictIds = new int[_numGroupByExpressions][];
+ _nullReplacedMultiValueDictIds = new int[_numGroupByExpressions][][];
+ } else {
+ _nullDictIds = null;
+ _nullReplacedSingleValueDictIds = null;
+ _nullReplacedMultiValueDictIds = null;
+ }
if (groupByExpressionSizesFromPredicates != null) {
Review Comment:
Routing null-handling queries into this generator exposes an old bug here.
`getOptimizedGroupByCardinality` shrinks the product, but raw keys still
compose from the full `_cardinalities`, so `ArrayBasedHolder` sizes `_flags`
too small.
Repro with `optimizeMaxInitialResultHolderCapacity=true`:
```sql
SELECT c0, c1, COUNT(*) FROM t WHERE c0 = <value with a high dict id> GROUP
BY c0, c1
```
On master it throws with `enableNullHandling=false` and works with `true`.
On this PR it throws in both:
```
ArrayIndexOutOfBoundsException: Index 99999 out of bounds for length 50
at DictionaryBasedGroupKeyGenerator$ArrayBasedHolder.markGroups
```
Not your bug, but null handling used to route around it. Can we skip
`ArrayBasedHolder` when the optimized product is in play?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java:
##########
@@ -118,6 +139,19 @@ public
DictionaryBasedGroupKeyGenerator(BaseProjectOperator<?> projectOperator,
_dictionaries[i] = columnContext.getDictionary();
assert _dictionaries[i] != null;
int cardinality = _dictionaries[i].length();
+ if (nullDictIds != null) {
+ // Reserve an id unless the column is known to produce no null. A
column read straight from a segment says so
+ // through its null value vector, which is the same vector a query
reads nulls from. A transform exposes no
+ // data source to consult and can still hand out a null bitmap of its
own, so it always reserves one.
+ DataSource dataSource = columnContext.getDataSource();
+ boolean tracksNulls = dataSource == null ||
dataSource.getNullValueVector() != null;
Review Comment:
This turns nullability into a plan-time decision. `FilteredGroupByOperator`
builds one shared generator from
`aggregationInfos.get(0).getProjectOperator()`, then feeds it blocks from the
other project operators.
I checked and it holds today: `StarTreeUtils` refuses star-tree when a
group-by column has nulls, so a star-tree/regular mix cannot reach here. Can
you note that? It is easy to break later.
##########
pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGeneratorTest.java:
##########
@@ -166,7 +166,7 @@ public void testArrayBasedSingleValue() {
DictionaryBasedGroupKeyGenerator dictionaryBasedGroupKeyGenerator =
new DictionaryBasedGroupKeyGenerator(_projectOperator,
getExpressions(groupByColumns),
Server.DEFAULT_QUERY_EXECUTOR_NUM_GROUPS_LIMIT,
- Server.DEFAULT_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY,
null);
+ Server.DEFAULT_QUERY_EXECUTOR_MAX_INITIAL_RESULT_HOLDER_CAPACITY,
false, null);
Review Comment:
Every call here passes `false`, so the new reserved-id path has no unit
test. The query tests cover it, but not `LongMapBasedHolder` or
`ArrayMapBasedHolder` with a reserved id. Worth one case with `true`.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/DictionaryBasedGroupKeyGenerator.java:
##########
@@ -216,25 +259,89 @@ public int getGlobalGroupKeyUpperBound() {
@Override
public void generateKeysForBlock(ValueBlock valueBlock, int[] groupKeys) {
// Fetch dictionary ids in the given block for all group-by columns
+ int numDocs = valueBlock.getNumDocs();
for (int i = 0; i < _numGroupByExpressions; i++) {
- BlockValSet blockValueSet =
valueBlock.getBlockValueSet(_groupByExpressions[i]);
- _singleValueDictIds[i] = blockValueSet.getDictionaryIdsSV();
+ _singleValueDictIds[i] =
getSingleValueDictIds(valueBlock.getBlockValueSet(_groupByExpressions[i]), i,
numDocs);
}
- _rawKeyHolder.processSingleValue(valueBlock.getNumDocs(), groupKeys);
+ _rawKeyHolder.processSingleValue(numDocs, groupKeys);
}
@Override
public void generateKeysForBlock(ValueBlock valueBlock, int[][] groupKeys) {
// Fetch dictionary ids in the given block for all group-by columns
+ int numDocs = valueBlock.getNumDocs();
for (int i = 0; i < _numGroupByExpressions; i++) {
BlockValSet blockValueSet =
valueBlock.getBlockValueSet(_groupByExpressions[i]);
if (_isSingleValueColumn[i]) {
- _singleValueDictIds[i] = blockValueSet.getDictionaryIdsSV();
+ _singleValueDictIds[i] = getSingleValueDictIds(blockValueSet, i,
numDocs);
} else {
- _multiValueDictIds[i] = blockValueSet.getDictionaryIdsMV();
+ _multiValueDictIds[i] = getMultiValueDictIds(blockValueSet, i,
numDocs);
}
}
- _rawKeyHolder.processMultiValue(valueBlock.getNumDocs(), groupKeys);
+ _rawKeyHolder.processMultiValue(numDocs, groupKeys);
+ }
+
+ /// Returns the block's single-value dictionary ids, with every null row
moved onto the column's reserved null
+ /// dictionary id so that nulls form one group of their own.
+ ///
+ /// The block owns the array it hands out and shares it with everything else
reading that column in this block, so
+ /// the ids are copied before being rewritten rather than replaced in place.
Returns the block's own array untouched
+ /// when the column reserves no null dictionary id or the block holds no
null.
+ private int[] getSingleValueDictIds(BlockValSet blockValSet, int index, int
numDocs) {
+ int[] dictIds = blockValSet.getDictionaryIdsSV();
+ if (_nullDictIds == null || _nullDictIds[index] < 0) {
+ return dictIds;
+ }
+ int nullDictId = _nullDictIds[index];
+ RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
+ if (nullBitmap == null || nullBitmap.isEmpty()) {
+ return dictIds;
+ }
+ assert _nullReplacedSingleValueDictIds != null;
+ int[] nullReplacedDictIds = _nullReplacedSingleValueDictIds[index];
+ if (nullReplacedDictIds == null || nullReplacedDictIds.length < numDocs) {
+ nullReplacedDictIds = new int[numDocs];
+ _nullReplacedSingleValueDictIds[index] = nullReplacedDictIds;
+ }
+ System.arraycopy(dictIds, 0, nullReplacedDictIds, 0, numDocs);
+ PeekableIntIterator nullIterator = nullBitmap.getIntIterator();
+ while (nullIterator.hasNext()) {
+ nullReplacedDictIds[nullIterator.next()] = nullDictId;
+ }
+ return nullReplacedDictIds;
+ }
+
+ /// Returns the block's multi-value dictionary ids, with every null row
replaced by a single entry holding the
+ /// column's reserved null dictionary id, so the row contributes one null
group instead of being read as the
+ /// column's default null value.
+ ///
+ /// Only the outer array is copied: the rows that are not null are handed on
pointing at the arrays the block
+ /// already gave out. Returns the block's own array untouched when the
column reserves no null dictionary id or the
+ /// block holds no null.
+ private int[][] getMultiValueDictIds(BlockValSet blockValSet, int index, int
numDocs) {
+ int[][] dictIds = blockValSet.getDictionaryIdsMV();
+ if (_nullDictIds == null || _nullDictIds[index] < 0) {
+ return dictIds;
+ }
+ int nullDictId = _nullDictIds[index];
+ RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
+ if (nullBitmap == null || nullBitmap.isEmpty()) {
+ return dictIds;
+ }
+ assert _nullReplacedMultiValueDictIds != null;
+ int[][] nullReplacedDictIds = _nullReplacedMultiValueDictIds[index];
+ if (nullReplacedDictIds == null || nullReplacedDictIds.length < numDocs) {
+ nullReplacedDictIds = new int[numDocs][];
+ _nullReplacedMultiValueDictIds[index] = nullReplacedDictIds;
+ }
+ System.arraycopy(dictIds, 0, nullReplacedDictIds, 0, numDocs);
+ PeekableIntIterator nullIterator = nullBitmap.getIntIterator();
+ while (nullIterator.hasNext()) {
+ // A fresh array per row rather than one shared between them: a raw key
holder writes its group ids back over
+ // the array it is handed for a row whose only group-by column is this
one
+ nullReplacedDictIds[nullIterator.next()] = new int[]{nullDictId};
Review Comment:
Can the description call this out? An empty MV array is ingested as null
(`NullValueColumnTransformer`), so `GROUP BY mvCol` on `[]` now returns a NULL
group. Postgres `unnest('{}')` returns no rows at all.
One NULL group beats the default null value by a mile, but it is a choice,
not the Postgres answer.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java:
##########
@@ -148,147 +152,85 @@ public void generateKeysForBlock(ValueBlock valueBlock,
int[] groupKeys) {
}
}
}
- int[] keyValues = new int[_numGroupByExpressions];
- // note that we are mutating its backing array for memory efficiency
- FixedIntArray flyweightKey = new FixedIntArray(keyValues);
+ // All null when null handling is disabled, so the per-column check below
never matches. The check stays inside
+ // the row loop rather than forking the loop per mode: every iteration
pays for a hash-map operation, which dwarfs
+ // a predicted branch.
+ RoaringBitmap[] nullBitmaps = new RoaringBitmap[_numGroupByExpressions];
if (_nullHandlingEnabled) {
- RoaringBitmap[] nullBitmaps = new RoaringBitmap[_numGroupByExpressions];
for (int i = 0; i < _numGroupByExpressions; i++) {
- nullBitmaps[i] =
valueBlock.getBlockValueSet(_groupByExpressions[i]).getNullBitmap();
+ RoaringBitmap nullBitmap =
valueBlock.getBlockValueSet(_groupByExpressions[i]).getNullBitmap();
+ // Normalize an empty bitmap to null so the per-row check is a plain
null comparison
+ nullBitmaps[i] = nullBitmap != null && !nullBitmap.isEmpty() ?
nullBitmap : null;
}
- for (int row = 0; row < numDocs; row++) {
- int numGroups = _groupKeyMap.size();
- boolean hasInvalidKeyValue = false;
- if (numGroups < _numGroupsLimit) {
- for (int col = 0; col < _numGroupByExpressions; col++) {
- if (nullBitmaps[col] != null && nullBitmaps[col].contains(row)) {
- keyValues[col] = ID_FOR_NULL;
- } else {
- Object columnValues = values[col];
- ValueToIdMap onTheFlyDictionary = _onTheFlyDictionaries[col];
- int keyValue;
- if (columnValues instanceof int[]) {
- keyValue = onTheFlyDictionary.put(((int[]) columnValues)[row]);
- } else if (columnValues instanceof long[]) {
- keyValue = onTheFlyDictionary.put(((long[])
columnValues)[row]);
- } else if (columnValues instanceof float[]) {
- keyValue = onTheFlyDictionary.put(((float[])
columnValues)[row]);
- } else if (columnValues instanceof double[]) {
- keyValue = onTheFlyDictionary.put(((double[])
columnValues)[row]);
- } else if (columnValues instanceof byte[][]) {
- keyValue = onTheFlyDictionary.put(new ByteArray(((byte[][])
columnValues)[row]));
- } else {
- keyValue = onTheFlyDictionary.put(((Object[])
columnValues)[row]);
- }
- keyValues[col] = keyValue;
- }
- }
- } else {
- for (int col = 0; col < _numGroupByExpressions; col++) {
- if (nullBitmaps[col] != null && nullBitmaps[col].contains(row)) {
- keyValues[col] = ID_FOR_NULL;
- } else {
- Object columnValues = values[col];
- ValueToIdMap onTheFlyDictionary = _onTheFlyDictionaries[col];
- int keyValue;
- if (columnValues instanceof int[]) {
- keyValue = onTheFlyDictionary.getId(((int[])
columnValues)[row]);
- } else if (columnValues instanceof long[]) {
- keyValue = onTheFlyDictionary.getId(((long[])
columnValues)[row]);
- } else if (columnValues instanceof float[]) {
- keyValue = onTheFlyDictionary.getId(((float[])
columnValues)[row]);
- } else if (columnValues instanceof double[]) {
- keyValue = onTheFlyDictionary.getId(((double[])
columnValues)[row]);
- } else if (columnValues instanceof byte[][]) {
- keyValue = onTheFlyDictionary.getId(new ByteArray(((byte[][])
columnValues)[row]));
- } else {
- keyValue = onTheFlyDictionary.getId(((Object[])
columnValues)[row]);
- }
- if (keyValue == INVALID_ID) {
- hasInvalidKeyValue = true;
- break;
- }
- }
- }
- }
- if (hasInvalidKeyValue) {
- groupKeys[row] = INVALID_ID;
- } else {
- int groupId = getGroupIdForKey(flyweightKey);
- if (groupId == numGroups) {
- // When a new group is added, create a new FixedIntArray
- keyValues = new int[_numGroupByExpressions];
- flyweightKey = new FixedIntArray(keyValues);
- }
- groupKeys[row] = groupId;
- }
- }
- } else {
- for (int row = 0; row < numDocs; row++) {
- int numGroups = _groupKeyMap.size();
- boolean hasInvalidKeyValue = false;
- if (numGroups < _numGroupsLimit) {
- for (int col = 0; col < _numGroupByExpressions; col++) {
+ }
+ int[] keyValues = new int[_numGroupByExpressions];
+ // note that we are mutating its backing array for memory efficiency
+ FixedIntArray flyweightKey = new FixedIntArray(keyValues);
+ for (int row = 0; row < numDocs; row++) {
+ int numGroups = _groupKeyMap.size();
+ boolean hasInvalidKeyValue = false;
+ if (numGroups < _numGroupsLimit) {
+ for (int col = 0; col < _numGroupByExpressions; col++) {
+ if (isNull(nullBitmaps[col], row)) {
+ keyValues[col] = ID_FOR_NULL;
+ } else {
Object columnValues = values[col];
ValueToIdMap onTheFlyDictionary = _onTheFlyDictionaries[col];
- int keyValue;
- if (columnValues instanceof int[]) {
- int columnValue = ((int[]) columnValues)[row];
- keyValue = onTheFlyDictionary != null ?
onTheFlyDictionary.put(columnValue) : columnValue;
- } else if (columnValues instanceof long[]) {
- keyValue = onTheFlyDictionary.put(((long[]) columnValues)[row]);
- } else if (columnValues instanceof float[]) {
- keyValue = onTheFlyDictionary.put(((float[]) columnValues)[row]);
- } else if (columnValues instanceof double[]) {
- keyValue = onTheFlyDictionary.put(((double[])
columnValues)[row]);
- } else if (columnValues instanceof byte[][]) {
- keyValue = onTheFlyDictionary.put(new ByteArray(((byte[][])
columnValues)[row]));
- } else {
- keyValue = onTheFlyDictionary.put(((Object[])
columnValues)[row]);
- }
- keyValues[col] = keyValue;
+ keyValues[col] = switch (columnValues) {
+ // A dictionary-encoded column has no on-the-fly dictionary: its
values are dictionary ids already, used
+ // as key values directly
+ case int[] ints -> onTheFlyDictionary != null ?
onTheFlyDictionary.put(ints[row]) : ints[row];
Review Comment:
With null handling on, line 86 sets `_dictionaries[i] = null` for every
column, so this `onTheFlyDictionary != null` branch is dead in that mode.
Nulls come from the bitmap now, and `buildKeysFromIds` already reads dict
ids. Can the `_nullHandlingEnabled ||` go, so a dict-encoded column beside a
raw one keeps the cheap path?
--
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]
