Jackie-Jiang commented on code in PR #19390:
URL: https://github.com/apache/pinot/pull/19390#discussion_r3911506786
##########
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:
Agreed, and thanks for chasing it down rather than filing it here — #19379
covers exactly this.
One thing worth carrying over to merge ordering: null handling was *routing
around* this bug on master, since those queries went to the no-dictionary
generators instead. This PR removes that accidental shield, so the failure
becomes reachable with `enableNullHandling=true` as well. #19379 should land
first, or the two together.
##########
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:
Good catch — this is the one I would not have found. Comment added at the
decision point in `f402575`:
```java
// This decides nullability once, from one project operator, while a shared
generator can be handed blocks
// from others: FilteredGroupByOperator builds a single generator for every
filtered aggregation. Those
// operators must therefore agree on which group-by columns can produce a
null. They do today, because a
// star-tree is only chosen for a null handling query when no column it
touches holds a null.
```
I verified your reading of the mechanism before writing it down: line 96
takes `aggregationInfos.get(0).getProjectOperator()`, and the loop threads that
one generator through every `AggregationInfo`, each with its own project
operator.
##########
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:
Added to the description under behavior changes, framed the way you put it —
one NULL group is not the Postgres answer, it is the closest one reachable
here, since Pinot ingests `[]` as null and a segment has no representation for
an empty multi-value row. So `[]` and SQL NULL are already indistinguishable by
the time a query reads them, and the choice is only between a NULL group and
the column's default null value.
##########
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:
Done in `f402575` — the selection is now just:
```java
Dictionary dictionary = columnContext.isDictionaryEncoded() ?
columnContext.getDictionary() : null;
```
I checked the four places that has to hold before making it: the `int[]`
arms already pass ids straight through when there is no on-the-fly dictionary;
`ID_FOR_NULL` is `-2`, so it cannot collide with a dictionary id;
`buildKeysFromIds` already branches on `_dictionaries[i] != null` when
rendering; and the at-limit `getId` path stays correct, because a dictionary id
is always a known value while group novelty is decided by `_groupKeyMap`. Class
javadoc now records that nullness is read from the bitmap rather than from the
id.
##########
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:
Added in `f402575`, though it needed the fixture to grow a nullable column
first: the segment had no nulls at all, so passing `true` would have reserved
no id and the test would have asserted nothing. There is now an all-null `n1`
column plus `setDefaultNullHandlingEnabled(true)`, so the null value vector is
actually written.
Three tests on top of that: `testArrayBasedNullHandling` asserts the upper
bound is `2` — one dictionary value plus the reserved id, which is the most
direct evidence the reservation happened — and `testLongMapBasedNullHandling` /
`testArrayMapBasedNullHandling` cover the two holders you named. Each asserts
every group key reads the column back as SQL `NULL`, and the map-based ones
assert their thread-local map is cleared on close.
Existing tests are unaffected: they pass their own column lists, and the
other columns still have no nulls, so nothing reserves an id for them.
--
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]
