Add api in AggregationFunction to get its compiled input expressions. #5339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mayankshriv
force-pushed
the
plan-cleanup
branch
2 times, most recently
from
37dc52c
to
f02f5f3
Compare
mayankshriv
changed the title
Jackie-Jiang
approved these changes
May 6, 2020
| columns.addAll(brokerRequest.getGroupBy().getExpressions()); | ||
| } | ||
| } else { | ||
| private void setMaxDocsForSelection(BrokerRequest brokerRequest) { |
There was a problem hiding this comment.
This logic should also be handle in the upper level (SelectionPlanNode) and passed to this class
There was a problem hiding this comment.
Yeah I thought so too. But upper levels can be aggregation as well, which will now have this field leaked. For now I tend to keep it here (as was the case before) until the full cleanup happens.
| private int _maxDocPerNextCall = DocIdSetPlanNode.MAX_DOC_PER_CALL; | ||
|
|
||
| public TransformPlanNode(IndexSegment indexSegment, BrokerRequest brokerRequest) { | ||
| public TransformPlanNode(IndexSegment indexSegment, BrokerRequest brokerRequest, | ||
| Set<TransformExpressionTree> expressionsToPlan) { |
There was a problem hiding this comment.
(nit) expressionsToPlan -> expressions?
Also pass maxDocsPerBlock from upper level?
There was a problem hiding this comment.
expressions was too generic and being used in too many places for different purposes, so I thought to call it expressionsToPlan.
Replied on maxDocsPerBlock above.
...core/src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunction.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/query/aggregation/function/AggregationFunctionUtils.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/query/aggregation/function/CountAggregationFunction.java
Outdated
Show resolved
Hide resolved
With aggregation functions now taking multiple agruments, only the functions themselves
have the knowledge on how to interpret these arguments. This poses a problem for the planning
phase on what columns need to be projected and what expressions need to be computed.
With this change, AggregationFunction's are now responsible for providing what inputs they need.
1. Added a new api in AggregationFunction interface `getInputExpressions()`, that returns a list
of compiled TransformExpressionTrees that the aggregation function needs as input to compute.
2. Cleaned up the chaining data dependency during planning phase. Before this PR, all planning nodes
receive the BrokerRequest (and pass to their child plan node) to extract out all information needed.
With this change:
- Aggregation plan nodes only specify the expression trees they need from Transform plan nodes, and
Transform plan nodes use that to specify what columns they need from projection plan nodes.
TODO: Ideally we should completely eliminate passing of BrokerRequest throughout the chain plan nodes,
and only pass minimal information instead. This change only does so for projection columns. A TODO here
is to extend it to FilterPlanNode and deeper.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With aggregation functions now taking multiple agruments, only the functions themselves
have the knowledge on how to interpret these arguments. This poses a problem for the planning
phase on what columns need to be projected and what expressions need to be computed.
With this change, AggregationFunction's are now responsible for providing what inputs they need.
Added a new api in AggregationFunction interface
getInputExpressions(), that returns a listof compiled TransformExpressionTrees that the aggregation function needs as input to compute.
Cleaned up the chaining data dependency during planning phase. Before this PR, all planning nodes
receive the BrokerRequest (and pass to their child plan node) to extract out all information needed.
With this change:
Transform plan nodes use that to specify what columns they need from projection plan nodes.
TODO: Ideally we should completely eliminate passing of BrokerRequest throughout the chain plan nodes,
and only pass minimal information instead. This change only does so for projection columns. A TODO here
is to extend it to FilterPlanNode and deeper.