-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Top n push down #110
Top n push down #110
Conversation
Refer to client PR: #178 |
case PhysicalOperation(projectList, filters, LogicalRelation(source: TiDBRelation, _, _)) | ||
if filters.forall(TiUtils.isSupportedFilter(_, source)) => | ||
pruneTopNFilterProject(limit, projectList, filters, source) :: Nil | ||
case TiAggregation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you please confirm if topN and aggregates can be pushdown independently.
For example, if you have group by name and count
you have
region 1: A, 100, B, 1
region 2: A, 100, B 1000
and you take Top 1. In above case, B will be ignored in region 1 request but actually B is the group to take and final result is 1000 instead of 1001.
I wonder if above case works in your implementation.
) | ||
} | ||
|
||
def validGroupAggregate(groupingExpressions: Seq[NamedExpression], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change it to isValidAggregates. Aggregates include aggexpr and groupby exprs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the real purpose for this PR? It seems for both refactor aggregates planning and topN.
If we don't need to do topN for aggregates. Do we still need to change all those stuff?
You might open an individual PR for refactoring if you see fit.
aggregate.AggUtils.planAggregateWithoutDistinct( | ||
) if validGroupAggregate(groupingExpressions, aggregateExpressions, source) => | ||
val dagReq: TiDAGRequest = filterToDAGRequest(filters, source) | ||
groupAggregateProjection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@ilovesoup Sorry for the inconvenience I bought, this PR only handles TopN logic but not aggregation with TopN logic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Integration test result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Support for
Limit
andorder by
clause.