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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
