adasari commented on code in PR #18334:
URL: https://github.com/apache/pinot/pull/18334#discussion_r3600301816
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -115,17 +115,38 @@ public Operator<AggregationResultsBlock>
buildNonFilteredAggOperator() {
boolean hasNullValues = _queryContext.isNullHandlingEnabled() &&
hasNullValues(aggregationFunctions);
if (!hasNullValues) {
+ DataSource[] dataSources = new DataSource[aggregationFunctions.length];
+ for (int i = 0; i < aggregationFunctions.length; i++) {
+ List<?> inputExpressions =
aggregationFunctions[i].getInputExpressions();
+ if (!inputExpressions.isEmpty()) {
+ String column = ((ExpressionContext)
inputExpressions.get(0)).getIdentifier();
+ dataSources[i] = _indexSegment.getDataSource(column,
_queryContext.getSchema());
+ }
+ }
+
// Priority 2: Check if non-scan based aggregation is feasible
if (filterOperator.isResultMatchingAll() && isFitForNonScanBasedPlan()) {
- DataSource[] dataSources = new DataSource[aggregationFunctions.length];
+ return new NonScanBasedAggregationOperator(_queryContext, dataSources,
numTotalDocs);
+ }
+
+ if (filterOperator.isResultMatchingAll()) {
+ boolean anyResolved = false;
+ Object[] preAggregatedResults = new
Object[aggregationFunctions.length];
for (int i = 0; i < aggregationFunctions.length; i++) {
- List<?> inputExpressions =
aggregationFunctions[i].getInputExpressions();
- if (!inputExpressions.isEmpty()) {
- String column = ((ExpressionContext)
inputExpressions.get(0)).getIdentifier();
- dataSources[i] = _indexSegment.getDataSource(column,
_queryContext.getSchema());
+ Object resolved =
AggregationFunctionUtils.getAggregationResult(aggregationFunctions[i],
+ dataSources[i], numTotalDocs,
NonScanBasedAggregationOperator.EXPLAIN_NAME);
+ if (resolved != null) {
+ preAggregatedResults[i] = resolved;
+ anyResolved = true;
}
}
- return new NonScanBasedAggregationOperator(_queryContext, dataSources,
numTotalDocs);
+
+ if (anyResolved) {
+ // build aggregation info for all functions (including those that
cannot be resolved from metadata)
+ aggregationInfo =
AggregationFunctionUtils.buildAggregationInfoWithoutStarTree(_segmentContext,
_queryContext,
Review Comment:
> buildAggregationInfoWithoutStarTree collects every function's columns, so
the metadata-resolved columns still get projected/scanned - we only skip the
per-row aggregate(), not the I/O. So the win is narrower than "avoid the scan".
Yes. The I/O required to read the dictionary which cannot be avoided.
However, aggregation over the full segment is skipped.
There is still room for further optimization. Some aggregation functions,
such as **MIN** and **MAX**, are still dictionary-based, but they can be
replaced with metadata-driven implementations.
In the future, we can also optimize the segment layout to minimize I/O. For
example, if dictionaries are confined to a small number of pages (from mmap),
it may be possible to avoid reading the actual index pages data altogether.
> Resolution runs here at plan-build time (dictionary scans, HLL merges),
whereas the full non-scan path does it lazily in getNextBlock(). Might be worth
deferring to keep planning cheap and the two paths consistent.
Done.
--
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]
