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: [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]
