You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/08/28 21:42:22 UTC
[PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Jackie-Jiang opened a new pull request, #19390:
URL: https://github.com/apache/pinot/pull/19390
## Summary
With null handling enabled, a group-by query was routed to the no-dictionary group key generators regardless of how its columns are encoded, and the multi-value key path ignored nulls entirely. This PR makes every group key generator null-aware, so the routing can go back to being decided by column encoding alone.
### Dictionary-based group key generation
With null handling enabled, a column whose segment tracks nulls reserves one further dictionary id, one past its last real id, to stand for a null value; its cardinality counts one more than the dictionary holds. A block's null rows are moved onto the reserved id before key composition (copying the block's id array, which is shared with other readers of the column), so the per-row kernels and the raw key arithmetic are untouched. The reserved id composes into the raw key like any other value and is read back out as SQL `NULL`.
A column that tracks no nulls reserves nothing, and neither does any column when null handling is disabled, which leaves the cardinalities, the holder selection and the block reads exactly as they are today. Dictionary-encoded group-by columns therefore keep the dictionary-based generator in both modes, instead of paying for per-row hash lookups on the no-dictionary path.
### Multi-value key path of the no-dictionary generators
The `int[][]` overloads of both no-dictionary generators read every column without consulting its null bitmap, so a null row was grouped under the column's default null value instead of under a group of its own — for every column on that path, single-value columns included. Both overloads now recognize nulls: a null row contributes one `NULL` group, mirroring the single-value behavior and the row's physical storage as a one-element default value.
With that closed, the null-enabled and null-disabled twin loops in both generators are merged: every iteration on these paths pays for a hash-map operation, which dwarfs the predicted per-row null check the merge adds, and both files end up smaller than before while doing more.
### Bugfix: rows misattributed once the group limit is reached
The null-enabled at-limit path of `NoDictionaryMultiColumnGroupKeyGenerator` resolved each column's key value but never stored it, composing whatever the key buffer held from the previous row — which, after a new group's buffer swap, is `{0, 0, ...}`: exactly the first group's key. With null handling enabled and `numGroupsLimit` reached, rows belonging to existing groups could be counted into the first group or dropped. The resolved value is now stored, same as the null-disabled twin always did.
### Behavior changes
All confined to queries that enable null handling:
- Grouping by a multi-value column with null rows now returns a `NULL` group; previously those rows were grouped under the column's default null value.
- Rows past the group limit are attributed to their correct existing groups (the bugfix above).
- The group id upper bound for dictionary-encoded columns is now derived from cardinalities rather than defaulting to `numGroupsLimit`, matching the null-handling-disabled behavior.
With null handling disabled nothing changes: no id is reserved, the routing condition is the encoding check it used to be, and the per-row loops of the dictionary-based generator are byte-identical.
### Tests
`NullHandlingEnabledQueriesTest` gains nine query-level tests covering every generator and key path with nulls: dictionary-encoded SV (including a row holding the column's default null value, which must not join the `NULL` group), two dictionary columns, dictionary MV, raw SV (INT and STRING, which exercise the primitive-map and object-map null keys respectively), raw MV, mixed raw/dictionary SV and MV, and a regression test for the group-limit misattribution that fails on the previous code by construction. `DictionaryBasedGroupKeyGeneratorTest` additionally covers `IntGroupIdMap.clearAndTrim` past the caching threshold.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19390:
URL: https://github.com/apache/pinot/pull/19390#issuecomment-5458563926
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19390?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `66.85714%` with `58 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.67%. Comparing base ([`1918041`](https://app.codecov.io/gh/apache/pinot/commit/19180414fdbc709b150a125156ce72cb1393921f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2b33578`](https://app.codecov.io/gh/apache/pinot/commit/2b335781ea69e387560b13554140414d7e652c9c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19390?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pby/NoDictionarySingleColumnGroupKeyGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Fgroupby%2FNoDictionarySingleColumnGroupKeyGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L05vRGljdGlvbmFyeVNpbmdsZUNvbHVtbkdyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | 40.98% | [28 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...upby/NoDictionaryMultiColumnGroupKeyGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Fgroupby%2FNoDictionaryMultiColumnGroupKeyGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L05vRGljdGlvbmFyeU11bHRpQ29sdW1uR3JvdXBLZXlHZW5lcmF0b3IuamF2YQ==) | 76.08% | [6 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tion/groupby/DictionaryBasedGroupKeyGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Fgroupby%2FDictionaryBasedGroupKeyGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L0RpY3Rpb25hcnlCYXNlZEdyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | 85.71% | [1 Missing and 8 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...egation/groupby/GroupingSetsGroupKeyGenerator.java](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fquery%2Faggregation%2Fgroupby%2FGroupingSetsGroupKeyGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9hZ2dyZWdhdGlvbi9ncm91cGJ5L0dyb3VwaW5nU2V0c0dyb3VwS2V5R2VuZXJhdG9yLmphdmE=) | 0.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19390?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (1918041) and HEAD (2b33578). Click for more details.
>
> <details><summary>HEAD has 24 uploads less than BASE</summary>
>
>| Flag | BASE (1918041) | HEAD (2b33578) |
>|------|------|------|
>|unittests|2|1|
>|java-25|6|1|
>|temurin|6|1|
>|lane-a|2|0|
>|integration|4|0|
>|integration2|2|0|
>|lane-b|2|0|
>|integration1|2|0|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19390 +/- ##
============================================
- Coverage 67.59% 57.67% -9.93%
+ Complexity 1430 1 -1429
============================================
Files 3486 2683 -803
Lines 224131 163947 -60184
Branches 35376 26611 -8765
============================================
- Hits 151497 94552 -56945
- Misses 60608 61387 +779
+ Partials 12026 8008 -4018
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.67% <66.85%> (-9.93%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.67% <66.85%> (-9.93%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.67% <66.85%> (-9.92%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.67% <66.85%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19390?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19390:
URL: https://github.com/apache/pinot/pull/19390#issuecomment-5505570506
All four applied in `f402575`. Thanks for the careful pass — the `FilteredGroupByOperator` one in particular I would not have found.
**Dictionary path for dict-encoded columns beside raw ones.** Done — the selection is now just `columnContext.isDictionaryEncoded() ? getDictionary() : null`. I checked the four places it has to hold: the `int[]` arms already pass ids 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`, 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 says nullness is read from the bitmap rather than from the id.
**Shared generator across project operators.** Comment added at the decision point, recording that `FilteredGroupByOperator` builds one generator and feeds it blocks from several project operators, that those operators must agree on which group-by columns can produce a null, and why they do today.
**Unit coverage with `true`.** This needed the fixture to grow a nullable column first — the segment had no nulls, so passing `true` would have reserved nothing and asserted nothing. Added an all-null `n1` plus `setDefaultNullHandlingEnabled(true)` so the null value vector is actually written, then three tests: `ArrayBased` (asserts the upper bound is `2` — one dictionary value plus the reserved id, which is the most direct evidence the reservation happened), and `LongMapBased` and `ArrayMapBased` as you asked. 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 untouched: they pass their own column lists and the other columns still have no nulls.
**Empty MV array.** Added to the description under behavior changes, framed as you put it — one NULL group is not the Postgres answer, it is the closest one reachable given that Pinot ingests `[]` as null and a segment cannot represent an empty multi-value row.
On the `optimizeMaxInitialResultHolderCapacity` interaction: agreed it belongs to #19379. Worth noting for merge order that null handling was routing *around* that bug on master, so this PR removes the shield — #19379 should land first, or together.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19390:
URL: https://github.com/apache/pinot/pull/19390#discussion_r3909660314
##########
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:
Hm actually nvm, this should be handled by https://github.com/apache/pinot/pull/19379
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19390:
URL: https://github.com/apache/pinot/pull/19390
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19390:
URL: https://github.com/apache/pinot/pull/19390#issuecomment-5458925597
Thanks — both confirmed and fixed in `c30ab5d`.
**1. Dictionary-backed transforms collapse NULL into the default value.** Correct, and the diagnosis is exact. `ColumnContext.fromTransformFunction` passes a `null` data source while setting `dictionaryEncoded` from the transform's own dictionary, and `TransformBlockValSet.getNullBitmap()` delegates to `BaseTransformFunction.getNullBitmap`, which ORs its arguments' bitmaps — so `filterMv` and the other dictionary-exposing transforms hand out nulls with no data source to inspect.
The nullability check is now conservative in the right direction: a column read from a segment can *prove* it has no nulls via its null value vector, so it still reserves nothing; anything without a data source reserves an id.
```java
boolean tracksNulls = dataSource == null || dataSource.getNullValueVector() != null;
```
Regression test: `testGroupByDictionaryBackedTransformKeepsNullApartFromTheDefaultNullValue`, grouping by `filterMv(col, 'v <= 1')` over rows holding `1`, NULL, and the INT default null value, asserting three distinct groups.
**2. Primitive null groups excluded from group accounting.** Confirmed, and both methods now return `_numGroups`, which counts map insertions and the external null group alike (for the object-keyed types the null key lives in the map, so the two were already equal).
One correction to the attribution, which matters for backporting: this is **pre-existing on master**, not new to the MV path. `getKeyForNullValue()` allocates outside `_groupKeyMap` in master's single-value path too (9 call sites), and master's `getNumKeys()` already returned `_groupKeyMap.size()`. The same `ArrayIndexOutOfBoundsException` reproduces on master with single-value data of that shape — this PR made it newly reachable from MV, which is presumably why it surfaced here.
Regression test: `testNullGroupIsCountedWhenSizingTheResultHolder`. Rather than the 10k-row block-boundary shape, it sets `maxInitialResultHolderCapacity=2` with three rows — `ensureCapacity` grows by `max(capacity * 2, capacity)`, so the under-reported `capacityNeeded=2` is a no-op against an initial capacity of 2 and the null group's id runs off the end. Same failure, three rows.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19390:
URL: https://github.com/apache/pinot/pull/19390#issuecomment-5458980900
Confirmed and fixed in `2cd84a8`.
`resolveColumnValueIds` delegates its single-value branch to the null-aware `resolveColumnIds`, but the multi-value branch resolved every element straight through the on-the-fly dictionary without consulting the null bitmap. Null rows now take the reserved null component, mirroring the other MV generators:
```java
if (_nullHandlingEnabled) {
RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
if (nullBitmap != null && !nullBitmap.isEmpty()) {
PeekableIntIterator nullIterator = nullBitmap.getIntIterator();
while (nullIterator.hasNext()) {
ids[nullIterator.next()] = _nullComponent;
}
}
}
```
Reusing the existing `_nullComponent` is safe and, I think, the right shape here: it already stands for a NULL key component when a grouping set excludes a column, it is already shared across every row and set, and `expandGroupIds` copies it into a fresh scratch array rather than writing through it. A column excluded from a set and a column whose value is NULL both render as NULL, which is why `GROUPING()` exists to tell them apart — so one component serving both roles matches the semantics. I extended the field's javadoc to say so.
Regression test: `testGroupingSetsOverMultiValueColumnHoldingNulls`, using your repro shape.
Same note as before on provenance: this one is also pre-existing on master — its MV resolver has no null-bitmap reference either. My "every group key generator is null-aware" wording in the description was wrong as written, since the grouping-sets MV path was outside the change; it is accurate now that this is fixed.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Support null handling in dictionary-based and multi-value group key generation [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19390:
URL: https://github.com/apache/pinot/pull/19390#issuecomment-5512996467
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1026 (merged).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org