dario-liberman commented on code in PR #18760:
URL: https://github.com/apache/pinot/pull/18760#discussion_r3465586921
##########
pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/funnel/AggregationStrategy.java:
##########
@@ -52,13 +56,38 @@ public AggregationStrategy(List<ExpressionContext>
stepExpressions, List<Express
_correlateByExpressions = correlateByExpressions;
_primaryCorrelationCol = _correlateByExpressions.get(0);
_numSteps = _stepExpressions.size();
+ _numCorrelateByKeys = _correlateByExpressions.size();
}
/**
- * Returns an aggregation result for this aggregation strategy to be kept in
a result holder (aggregation only).
+ * Creates an aggregation result for single-key correlation.
*/
abstract A createAggregationResult(Dictionary dictionary);
+ /**
+ * Creates an aggregation result for multi-key correlation.
+ */
+ abstract A createAggregationResultMultiKey(Dictionary[] dictionaries);
Review Comment:
I think we can start with this for now, but it would be better in my opinion
if we do not have a physical column requirement on secondary correlation keys.
I think for the primary correlation key it makes sense that we require it to
be a simple dictionary encoded column, but for secondary correlation I believe
we could allow for any expression including projections.
Again, I think it is fine to start with this restriction for this pull
request, but would be great to follow-up with a generalization.
--
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]
