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/26 00:59:35 UTC
[PR] Add null-aware star-tree index [pinot]
Jackie-Jiang opened a new pull request, #19367:
URL: https://github.com/apache/pinot/pull/19367
## Summary
Adds a second star-tree variant that pre-aggregates with null-handling-on semantics, selected with a new `nullHandlingEnabled` flag on `StarTreeIndexConfig`. Today a star-tree folds nulls into the column's default value and counts them, which is right for a null-handling-off query and wrong for a null-handling-on one — so a query that enables null handling can only use a star-tree when nothing it touches is actually null, and otherwise drops to a raw scan.
Both variants can live in the same segment. The flag is part of the builder config's identity, so they survive dedup and one is never reused for the other.
Depends on #19218: excluding nulls from the pre-aggregation only agrees with a raw scan once the aggregation functions themselves skip null rows, which is now true of all of them.
### Dimensions
Nulls are stored under a reserved dictionary id — the column's cardinality, one past the last real id — so null rows form their own tree node instead of collapsing into the default value's node. The forward index is widened to `cardinality + 1` values and `StarTreeLoaderUtils` recomputes the bit width to match.
A null vector is written for dimensions as well as metrics: the reserved id is out of range for the segment dictionary the star-tree shares, so the query side has to recognise a null from the vector rather than from the stored id.
### Metrics
Null values are excluded from the pre-aggregation. A group left with no non-null input has no aggregated value to store, so the metric keeps a placeholder in the forward index and its nullness lives in the metric's null vector — the same shape a regular column is stored in.
`COUNT` is the one aggregator that still answers for such a group itself, through `ValueAggregator#getAllNullAggregatedValue`. It is read back by summing the pre-aggregated column rather than through the null vector, and `0` is exactly the placeholder a null would leave behind, so recording the group in the vector would cost a bit per group and buy nothing. Every other aggregator takes the `null` default, which keeps an all-null group down to a placeholder plus one null-vector bit rather than a serialized empty sketch, and stops `getMaxAggregatedValueByteSize` sizing the column for one.
`COUNT(column)` becomes representable as a stored function-column pair so a null-aware tree can pre-aggregate per-column non-null counts. Config parsing keeps the previous `COUNT__col` → `count__*` normalisation for regular trees, so existing table configs are unaffected.
### The off-heap builder
The on-heap builder holds `Record`s in memory, where a null metric needs no encoding. The off-heap builder serializes every record to a temporary store and could not express one — it dereferenced the missing value while serializing. Nullness is now held in a per-metric `RoaringBitmap` keyed by doc id, and the serialized record carries the aggregated type's zero in its place.
Keying on the doc id is safe because records never move: sorting permutes an array of doc ids and compares through it, leaving each record where it was written. The record layout is unchanged, so `FixedSizeRecordOffsets`' arithmetic still holds.
### Query routing
Each query is routed to a tree built in the matching mode. A null-handling-on query may still fall back to a regular tree when none of the columns it touches contains a null value — that is the check that already existed for exactly this case, now extracted so both paths share it.
The resolved function-column pairs travel with the project operator through `AggregationInfo`, because `COUNT` resolves differently per tree and the executors must read back the same columns that were projected. `AggregationInfo` derives `isUseStarTree()` from whether those pairs are present rather than carrying a separate flag, so the two cannot disagree.
### Compatibility
A deployment that does not opt in is unaffected: an old segment has no `null.handling.enabled` property and reads `false`, an old table config defaults the flag to `false`, and the new mode filter skips nothing when both are `false`. Same trees built, same tree chosen, same answers.
The behavioural change is confined to segments that opt into a null-aware tree, where a null-handling-on query now gets the same answer a raw scan would give instead of falling back to one.
### Tests
`NullAwareStarTreeBuilderTest` builds a segment whose metric is null for a whole dimension group and covers both that the all-null group survives the build and that the two variants coexist and disagree — a regular tree reports the column default as its minimum and has no null vector, a null-aware one excludes those rows and does.
The build mode is a `@DataProvider` parameter rather than a random choice. `BaseStarTreeV2Test` picks between `ON_HEAP` and `OFF_HEAP` with `RANDOM.nextBoolean()` across all its subclasses, which would have made the off-heap gap above a coin-flip flake rather than a failure.
--
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] Add null-aware star-tree index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #19367:
URL: https://github.com/apache/pinot/pull/19367#issuecomment-5722969422
@yashmayya Thanks, both were real, and both turned out to be general null-handling gaps rather than star-tree ones, so they were fixed on master first and this PR is now rebased on them:
- **Grouping by a null dimension.** #19390 makes null-handling group-by use `DictionaryBasedGroupKeyGenerator` for dictionary-encoded columns, with `dictionary.length()` as the reserved null id. That is exactly the id this tree stores, so the reserved id no longer reaches the dictionary at all.
- **Always-true predicate.** #19440 keeps an always-true predicate on a column with nulls instead of dropping it, in the leaf filter and in `StarTreeUtils`. The column reaches the null check, and on a null-aware tree the matching ids are bounded by the dictionary, so the null child is never traversed.
The two tests that reproduced them stay in `NullAwareStarTreeQueriesTest`, which now also covers exclusive and negated predicates, a filtered group-by, an all-null metric group, both build modes, and routing between the two variants in one segment. Ready for another look.
--
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] Add null-aware star-tree index [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #19367:
URL: https://github.com/apache/pinot/pull/19367#discussion_r4049769554
##########
pinot-core/src/main/java/org/apache/pinot/core/startree/StarTreeUtils.java:
##########
@@ -405,82 +449,117 @@ public static BaseProjectOperator<?> createStarTreeBasedProjectOperator(IndexSeg
.toArray(new ExpressionContext[0]) : null;
if (queryContext.isNullHandlingEnabled()) {
- // We can still use the star-tree index if there aren't actually any null values in this segment for all the
- // metrics being aggregated, all the dimensions being filtered on / grouped by.
- for (int i = 0; i < aggregationFunctionColumnPairs.length; i++) {
- AggregationFunctionColumnPair aggregationFunctionColumnPair = aggregationFunctionColumnPairs[i];
- if (aggregationFunctionColumnPair == AggregationFunctionColumnPair.COUNT_STAR) {
- // COUNT aggregation function returns a non-empty input expressions list only when null handling is enabled
- // and the input operand is a non-star identifier or function.
- List<ExpressionContext> inputExpressions = aggregationFunctions[i].getInputExpressions();
- if (!inputExpressions.isEmpty()) {
- if (inputExpressions.get(0).getType() == ExpressionContext.Type.IDENTIFIER) {
- DataSource dataSource = indexSegment.getDataSource(inputExpressions.get(0).getIdentifier());
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- return null;
- }
- }
- }
- // Null handling is irrelevant for COUNT(*), COUNT(literal), COUNT(nonNullColumn)
- continue;
- }
-
- String column = aggregationFunctionColumnPair.getColumn();
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because aggregation column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because aggregation column: '{}' has null values", column);
- return null;
- }
- }
-
- for (String column : predicateEvaluatorsMap.keySet()) {
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because filter column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because filter column: '{}' has null values", column);
- return null;
- }
+ // A null-aware star-tree pre-aggregates with exactly the semantics the query asks for
+ StarTreeProjectPlan plan = createProjectPlan(indexSegment, queryContext, starTrees, true, aggregationFunctions,
+ groupByExpressions, predicateEvaluatorsMap);
+ if (plan != null) {
+ return plan;
}
+ }
+ return createProjectPlan(indexSegment, queryContext, starTrees, false, aggregationFunctions, groupByExpressions,
+ predicateEvaluatorsMap);
+ }
- Set<String> groupByColumns = new HashSet<>();
- if (groupByExpressions != null) {
- for (ExpressionContext groupByExpression : groupByExpressions) {
- groupByExpression.getColumns(groupByColumns);
- }
- }
- for (String column : groupByColumns) {
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because group-by column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because group-by column: '{}' has null values", column);
- return null;
- }
- }
+ /// Returns a [StarTreeProjectPlan] built on the first star-tree that both matches `nullAware` and fits the query,
+ /// or `null` if there is none.
+ ///
+ /// Resolves the function-column pairs against the same mode, because a null-aware star-tree stores `COUNT` per
+ /// column while a regular one stores a single count of every row, and the executors have to read back whichever
+ /// was projected.
+ @Nullable
+ private static StarTreeProjectPlan createProjectPlan(IndexSegment indexSegment, QueryContext queryContext,
+ List<StarTreeV2> starTrees, boolean nullAware, AggregationFunction[] aggregationFunctions,
+ @Nullable ExpressionContext[] groupByExpressions,
+ Map<String, List<CompositePredicateEvaluator>> predicateEvaluatorsMap) {
+ // Only `COUNT` resolves differently between the two, and never to `null`, so a query that cannot be represented
+ // as pairs at all fails here for either kind of star-tree
+ AggregationFunctionColumnPair[] functionColumnPairs =
+ extractAggregationFunctionPairs(aggregationFunctions, nullAware);
+ if (functionColumnPairs == null) {
+ return null;
+ }
+ // A regular star-tree folded nulls into the column's default value and counted them, so it can only answer a
+ // null-handling-on query when nothing the query touches is actually null
+ if (!nullAware && queryContext.isNullHandlingEnabled() && !hasNoNullValues(indexSegment, aggregationFunctions,
+ functionColumnPairs, predicateEvaluatorsMap.keySet(), groupByExpressions)) {
+ return null;
}
List<Pair<AggregationFunction, AggregationFunctionColumnPair>> aggregations =
new ArrayList<>(aggregationFunctions.length);
for (int i = 0; i < aggregationFunctions.length; i++) {
- aggregations.add(Pair.of(aggregationFunctions[i], aggregationFunctionColumnPairs[i]));
+ aggregations.add(Pair.of(aggregationFunctions[i], functionColumnPairs[i]));
}
for (StarTreeV2 starTreeV2 : starTrees) {
- if (isFitForStarTree(starTreeV2.getMetadata(), aggregations, groupByExpressions,
- predicateEvaluatorsMap.keySet())) {
- return new StarTreeProjectPlanNode(queryContext, starTreeV2, aggregationFunctionColumnPairs, groupByExpressions,
- predicateEvaluatorsMap).run();
+ StarTreeV2Metadata metadata = starTreeV2.getMetadata();
+ if (metadata.isNullHandlingEnabled() != nullAware) {
+ continue;
+ }
+ if (isFitForStarTree(metadata, aggregations, groupByExpressions, predicateEvaluatorsMap.keySet())) {
Review Comment:
`isFitForStarTree` only checks that the group-by *columns* are tree dimensions, so `GROUP BY d + 1` is accepted. The transform then reads `d`'s values and hands the reserved null id to the shared dictionary:
```
SELECT d + 1, SUM(m) FROM testTable GROUP BY d + 1
star-tree: IndexOutOfBoundsException in the dictionary read
scan: 3 rows
```
Data dependent: it passes once a filter removes the null rows, so the same query works on one segment and fails on another.
Either refuse a non-identifier group-by on a null-aware tree, or resolve the reserved id inside the star-tree block.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -41,6 +41,28 @@ public interface ValueAggregator<R, A> {
/// specified in the schema.
A getInitialAggregatedValue(@Nullable R rawValue);
+ /// Returns the aggregated value of a group whose input values are all null, or `null` to have the star-tree record
+ /// the group in its null vector instead.
+ ///
+ /// Only consulted by null-aware star-trees, which exclude null input values from the pre-aggregation and can
+ /// therefore produce a group with no values at all.
+ ///
+ /// Returning `null` is safe whenever the aggregation function skips null rows while reading the pre-aggregated
+ /// column, which every aggregation function does apart from `COUNT`. A group recorded in the null vector is never
Review Comment:
Not true of `AVG`. Its three star-tree paths (`aggregateSerialized`, `aggregateGroupBySVSerialized`, `aggregateGroupByMVSerialized`) loop `0..length` and deserialize every row, with no `forEachNotNull`. So they read the empty placeholder and throw. `AVG_MV` inherits it.
Repro using this PR's own test, with `AVG__m2` added to the null-aware config:
```
SELECT d, AVG(m2) FROM testTable GROUP BY d
star-tree: BufferUnderflowException at AvgPair.fromByteBuffer
scan: [1, 26.67] [2, null] [null, 50.0]
```
Every other `BYTES` aggregator wraps the loop. Simplest fix: override `getAllNullAggregatedValue()` in `AvgValueAggregator` to return an empty `AvgPair`, like `CountValueAggregator` does.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -41,6 +41,28 @@ public interface ValueAggregator<R, A> {
/// specified in the schema.
A getInitialAggregatedValue(@Nullable R rawValue);
+ /// Returns the aggregated value of a group whose input values are all null, or `null` to have the star-tree record
+ /// the group in its null vector instead.
+ ///
+ /// Only consulted by null-aware star-trees, which exclude null input values from the pre-aggregation and can
+ /// therefore produce a group with no values at all.
+ ///
+ /// Returning `null` is safe whenever the aggregation function skips null rows while reading the pre-aggregated
+ /// column, which every aggregation function does apart from `COUNT`. A group recorded in the null vector is never
+ /// read back, so the placeholder left in the forward index is never deserialized.
+ ///
+ /// `COUNT` is the exception and overrides this: it is read back by summing the pre-aggregated column rather than
+ /// through the null vector, so it answers `0` itself. Every other aggregator takes the default, which keeps an
+ /// all-null group down to a placeholder plus one null-vector bit instead of a serialized empty sketch.
+ ///
+ /// An aggregator whose [#getAggregatedValueType] is `BYTES` must also make [#getMaxAggregatedValueByteSize] account
Review Comment:
Nothing meets this contract today, and `SUMPRECISION` now fails the build because of it.
`_maxByteSize` only grows while values are aggregated. A null-aware tree aggregates nothing for an all-null column, so it stays 0 and this throws:
```
IllegalStateException: Unknown max aggregated value byte size, please provide maximum precision as the second argument
at SumPrecisionValueAggregator.getMaxAggregatedValueByteSize
at BaseSingleTreeBuilder.createForwardIndexes
```
The same config builds fine with the flag off. The message is also misleading — precision is not the problem.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -59,20 +74,37 @@ public static String toColumnName(AggregationFunctionType functionType, String c
}
public static AggregationFunctionColumnPair fromColumnName(String columnName) {
+ return fromColumnName(columnName, false);
+ }
+
+ /// Parses a function-column pair name such as `sum__col`.
+ ///
+ /// When `preserveCountColumn` is `false`, `count__col` resolves to [#COUNT_STAR], matching how a regular star-tree
+ /// stores counts. Pass `true` for a null-aware star-tree, where `count__col` denotes the non-null count of `col`.
+ public static AggregationFunctionColumnPair fromColumnName(String columnName, boolean preserveCountColumn) {
String[] parts = columnName.split(DELIMITER, 2);
- return fromFunctionAndColumnName(parts[0], parts[1]);
+ return fromFunctionAndColumnName(parts[0], parts[1], preserveCountColumn);
}
- public static AggregationFunctionColumnPair fromAggregationConfig(StarTreeAggregationConfig aggregationConfig) {
- return fromFunctionAndColumnName(aggregationConfig.getAggregationFunction(), aggregationConfig.getColumnName());
+ /// Builds a pair from an aggregation config. See [#fromColumnName] for the meaning of `preserveCountColumn`.
+ public static AggregationFunctionColumnPair fromAggregationConfig(StarTreeAggregationConfig aggregationConfig,
Review Comment:
`fromColumnName(String)` kept a 1-arg overload just above. This one did not, which is the second half of the red binary-compat check and is not mentioned in the description.
Adding `fromAggregationConfig(config)` delegating to `(config, false)` leaves only the `writeMetadata` break you already call out.
--
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] Add null-aware star-tree index [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19367:
URL: https://github.com/apache/pinot/pull/19367#issuecomment-5419585362
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19367?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`0513b74`](https://app.codecov.io/gh/apache/pinot/commit/0513b7413439f5d5a5c4318a6cbec93e0b9b398e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8d3ac3d`](https://app.codecov.io/gh/apache/pinot/commit/8d3ac3d9e270820bf0a3f16eefc378d704ba59f7?dropdown=coverage&el=desc&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 (0513b74) and HEAD (8d3ac3d). Click for more details.
>
> <details><summary>HEAD has 18 uploads less than BASE</summary>
>
>| Flag | BASE (0513b74) | HEAD (8d3ac3d) |
>|------|------|------|
>|unittests1|1|0|
>|unittests|2|0|
>|java-25|6|2|
>|temurin|6|2|
>|lane-a|2|1|
>|integration|4|2|
>|integration1|2|0|
>|lane-b|2|1|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19367 +/- ##
=============================================
- Coverage 67.44% 0.00% -67.45%
=============================================
Files 3485 3 -3482
Lines 223874 6 -223868
Branches 35300 0 -35300
=============================================
- Hits 150988 0 -150988
+ Misses 60895 6 -60889
+ Partials 11991 0 -11991
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19367/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/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19367/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/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.45%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-100.00%)` | :arrow_down: |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (-67.45%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19367/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19367/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/19367?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] Add null-aware star-tree index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19367:
URL: https://github.com/apache/pinot/pull/19367
--
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] Add null-aware star-tree index [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19367:
URL: https://github.com/apache/pinot/pull/19367#issuecomment-5770079853
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1056
--
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] Add null-aware star-tree index [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #19367:
URL: https://github.com/apache/pinot/pull/19367#discussion_r4051401996
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -41,6 +41,28 @@ public interface ValueAggregator<R, A> {
/// specified in the schema.
A getInitialAggregatedValue(@Nullable R rawValue);
+ /// Returns the aggregated value of a group whose input values are all null, or `null` to have the star-tree record
+ /// the group in its null vector instead.
+ ///
+ /// Only consulted by null-aware star-trees, which exclude null input values from the pre-aggregation and can
+ /// therefore produce a group with no values at all.
+ ///
+ /// Returning `null` is safe whenever the aggregation function skips null rows while reading the pre-aggregated
+ /// column, which every aggregation function does apart from `COUNT`. A group recorded in the null vector is never
Review Comment:
Fixed, though not the way you suggested — an empty `AvgPair` from `AvgValueAggregator` stops the crash but answers the wrong thing. `extractFinalResult` renders a zero-count pair as `DEFAULT_FINAL_RESULT`, i.e. `Double.NEGATIVE_INFINITY`, so the all-null group would come back as `-Infinity` instead of `NULL`. `CountValueAggregator` can override precisely because `0` is genuinely `COUNT`'s answer for an empty group; `AVG` has no such value.
The gap was on the reading side, so that is where it is fixed: all three serialized paths now wrap their loop in `forEachNotNull`, like every other `BYTES` aggregator, and `AVG_MV` inherits it.
Your repro is now a test — `anAllNullMetricGroupAveragesToNull`, with `AVG__m2` in the null-aware config exactly as you had it.
I also expanded the contract here to spell out when returning `null` is safe, since "every aggregation function skips null rows while reading the pre-aggregated column" was the unstated assumption that `AVG` broke.
##########
pinot-core/src/main/java/org/apache/pinot/core/startree/StarTreeUtils.java:
##########
@@ -405,82 +449,117 @@ public static BaseProjectOperator<?> createStarTreeBasedProjectOperator(IndexSeg
.toArray(new ExpressionContext[0]) : null;
if (queryContext.isNullHandlingEnabled()) {
- // We can still use the star-tree index if there aren't actually any null values in this segment for all the
- // metrics being aggregated, all the dimensions being filtered on / grouped by.
- for (int i = 0; i < aggregationFunctionColumnPairs.length; i++) {
- AggregationFunctionColumnPair aggregationFunctionColumnPair = aggregationFunctionColumnPairs[i];
- if (aggregationFunctionColumnPair == AggregationFunctionColumnPair.COUNT_STAR) {
- // COUNT aggregation function returns a non-empty input expressions list only when null handling is enabled
- // and the input operand is a non-star identifier or function.
- List<ExpressionContext> inputExpressions = aggregationFunctions[i].getInputExpressions();
- if (!inputExpressions.isEmpty()) {
- if (inputExpressions.get(0).getType() == ExpressionContext.Type.IDENTIFIER) {
- DataSource dataSource = indexSegment.getDataSource(inputExpressions.get(0).getIdentifier());
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- return null;
- }
- }
- }
- // Null handling is irrelevant for COUNT(*), COUNT(literal), COUNT(nonNullColumn)
- continue;
- }
-
- String column = aggregationFunctionColumnPair.getColumn();
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because aggregation column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because aggregation column: '{}' has null values", column);
- return null;
- }
- }
-
- for (String column : predicateEvaluatorsMap.keySet()) {
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because filter column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because filter column: '{}' has null values", column);
- return null;
- }
+ // A null-aware star-tree pre-aggregates with exactly the semantics the query asks for
+ StarTreeProjectPlan plan = createProjectPlan(indexSegment, queryContext, starTrees, true, aggregationFunctions,
+ groupByExpressions, predicateEvaluatorsMap);
+ if (plan != null) {
+ return plan;
}
+ }
+ return createProjectPlan(indexSegment, queryContext, starTrees, false, aggregationFunctions, groupByExpressions,
+ predicateEvaluatorsMap);
+ }
- Set<String> groupByColumns = new HashSet<>();
- if (groupByExpressions != null) {
- for (ExpressionContext groupByExpression : groupByExpressions) {
- groupByExpression.getColumns(groupByColumns);
- }
- }
- for (String column : groupByColumns) {
- DataSource dataSource = indexSegment.getDataSourceNullable(column);
- if (dataSource == null) {
- LOGGER.debug("Cannot use star-tree index because group-by column: '{}' does not exist", column);
- return null;
- }
- if (FilterOperatorUtils.hasNulls(dataSource)) {
- LOGGER.debug("Cannot use star-tree index because group-by column: '{}' has null values", column);
- return null;
- }
- }
+ /// Returns a [StarTreeProjectPlan] built on the first star-tree that both matches `nullAware` and fits the query,
+ /// or `null` if there is none.
+ ///
+ /// Resolves the function-column pairs against the same mode, because a null-aware star-tree stores `COUNT` per
+ /// column while a regular one stores a single count of every row, and the executors have to read back whichever
+ /// was projected.
+ @Nullable
+ private static StarTreeProjectPlan createProjectPlan(IndexSegment indexSegment, QueryContext queryContext,
+ List<StarTreeV2> starTrees, boolean nullAware, AggregationFunction[] aggregationFunctions,
+ @Nullable ExpressionContext[] groupByExpressions,
+ Map<String, List<CompositePredicateEvaluator>> predicateEvaluatorsMap) {
+ // Only `COUNT` resolves differently between the two, and never to `null`, so a query that cannot be represented
+ // as pairs at all fails here for either kind of star-tree
+ AggregationFunctionColumnPair[] functionColumnPairs =
+ extractAggregationFunctionPairs(aggregationFunctions, nullAware);
+ if (functionColumnPairs == null) {
+ return null;
+ }
+ // A regular star-tree folded nulls into the column's default value and counted them, so it can only answer a
+ // null-handling-on query when nothing the query touches is actually null
+ if (!nullAware && queryContext.isNullHandlingEnabled() && !hasNoNullValues(indexSegment, aggregationFunctions,
+ functionColumnPairs, predicateEvaluatorsMap.keySet(), groupByExpressions)) {
+ return null;
}
List<Pair<AggregationFunction, AggregationFunctionColumnPair>> aggregations =
new ArrayList<>(aggregationFunctions.length);
for (int i = 0; i < aggregationFunctions.length; i++) {
- aggregations.add(Pair.of(aggregationFunctions[i], aggregationFunctionColumnPairs[i]));
+ aggregations.add(Pair.of(aggregationFunctions[i], functionColumnPairs[i]));
}
for (StarTreeV2 starTreeV2 : starTrees) {
- if (isFitForStarTree(starTreeV2.getMetadata(), aggregations, groupByExpressions,
- predicateEvaluatorsMap.keySet())) {
- return new StarTreeProjectPlanNode(queryContext, starTreeV2, aggregationFunctionColumnPairs, groupByExpressions,
- predicateEvaluatorsMap).run();
+ StarTreeV2Metadata metadata = starTreeV2.getMetadata();
+ if (metadata.isNullHandlingEnabled() != nullAware) {
+ continue;
+ }
+ if (isFitForStarTree(metadata, aggregations, groupByExpressions, predicateEvaluatorsMap.keySet())) {
Review Comment:
Good catch, and the data dependence makes it worse than it first looks. Rather than refuse the transform, I changed the encoding so the read works.
The reserved id now exists only inside the tree. The forward index stores the column's default null value and marks the row in the dimension's null vector, which is exactly how a regular column stores a null, so a value-based read of a star-tree dimension resolves through the shared segment dictionary like any other column.
The reserved id was never doing any work at query time in the first place — the group key generator takes the null group from the null vector, not from the stored id — so nothing is lost by not storing it, and the `cardinality + 1` widening plus the `StarTreeLoaderUtils` bit-width recomputation both go away with it.
`groupingByATransformOverANullDimensionReturnsTheNullGroup` runs your query.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/ValueAggregator.java:
##########
@@ -41,6 +41,28 @@ public interface ValueAggregator<R, A> {
/// specified in the schema.
A getInitialAggregatedValue(@Nullable R rawValue);
+ /// Returns the aggregated value of a group whose input values are all null, or `null` to have the star-tree record
+ /// the group in its null vector instead.
+ ///
+ /// Only consulted by null-aware star-trees, which exclude null input values from the pre-aggregation and can
+ /// therefore produce a group with no values at all.
+ ///
+ /// Returning `null` is safe whenever the aggregation function skips null rows while reading the pre-aggregated
+ /// column, which every aggregation function does apart from `COUNT`. A group recorded in the null vector is never
+ /// read back, so the placeholder left in the forward index is never deserialized.
+ ///
+ /// `COUNT` is the exception and overrides this: it is read back by summing the pre-aggregated column rather than
+ /// through the null vector, so it answers `0` itself. Every other aggregator takes the default, which keeps an
+ /// all-null group down to a placeholder plus one null-vector bit instead of a serialized empty sketch.
+ ///
+ /// An aggregator whose [#getAggregatedValueType] is `BYTES` must also make [#getMaxAggregatedValueByteSize] account
Review Comment:
Fixed. The builder now tracks whether a metric aggregated anything at all, and sizes the forward index from the serialized length of `getAllNullAggregatedValue()` when it did not, falling back to zero when there is no such value.
`getMaxAggregatedValueByteSize()` is therefore only consulted for a column that actually has values, which leaves `SumPrecisionValueAggregator`'s precondition intact — realtime ingestion aggregation still relies on it, so the fix belongs at the star-tree call site rather than in the aggregator.
I also wrote the obligation into the interface contract: an aggregator whose `getAggregatedValueType()` is `BYTES` has to make `getMaxAggregatedValueByteSize` account for its all-null value, or the star-tree under-allocates. `anAllNullMetricColumnAggregatesToNull` covers a fully null `BIG_DECIMAL` column under `SUMPRECISION`.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/startree/AggregationFunctionColumnPair.java:
##########
@@ -59,20 +74,37 @@ public static String toColumnName(AggregationFunctionType functionType, String c
}
public static AggregationFunctionColumnPair fromColumnName(String columnName) {
+ return fromColumnName(columnName, false);
+ }
+
+ /// Parses a function-column pair name such as `sum__col`.
+ ///
+ /// When `preserveCountColumn` is `false`, `count__col` resolves to [#COUNT_STAR], matching how a regular star-tree
+ /// stores counts. Pass `true` for a null-aware star-tree, where `count__col` denotes the non-null count of `col`.
+ public static AggregationFunctionColumnPair fromColumnName(String columnName, boolean preserveCountColumn) {
String[] parts = columnName.split(DELIMITER, 2);
- return fromFunctionAndColumnName(parts[0], parts[1]);
+ return fromFunctionAndColumnName(parts[0], parts[1], preserveCountColumn);
}
- public static AggregationFunctionColumnPair fromAggregationConfig(StarTreeAggregationConfig aggregationConfig) {
- return fromFunctionAndColumnName(aggregationConfig.getAggregationFunction(), aggregationConfig.getColumnName());
+ /// Builds a pair from an aggregation config. See [#fromColumnName] for the meaning of `preserveCountColumn`.
+ public static AggregationFunctionColumnPair fromAggregationConfig(StarTreeAggregationConfig aggregationConfig,
Review Comment:
The description gap was real and is fixed — the compatibility section now lists every API break and says which two are what leave the binary-compatibility check red.
I am keeping the signature change rather than adding the delegating overload, though. `fromColumnName` kept its one-argument form because it has a genuine mode-free meaning (reading a pair name off an old segment) and callers outside this repo. `fromAggregationConfig` has two callers, both in-tree, and the pair it builds is only correct for one tree mode — so a one-argument form would be an overload whose only remaining purpose is to let a caller silently build the wrong pair.
The check stays red regardless: `writeMetadata` breaks too, and, as the description now spells out, a null-aware tree cannot be read correctly by a server predating this change at all, so enabling the flag already requires a full cluster upgrade.
--
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