-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[spark] Support push down aggregate #4259
Conversation
cc @JingsongLi thank you |
} | ||
} | ||
|
||
def supportAggregation(aggregation: Aggregation): Boolean = { |
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.
There is an initialize
in this supportAggregation
. It looks like not a support
. Can you rename this to pushAggregation
?
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.
+1, changed to pushAggregation
@@ -38,12 +38,12 @@ abstract class PaimonBaseScanBuilder(table: Table) | |||
|
|||
protected var pushed: Array[(Filter, Predicate)] = Array.empty |
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.
Can we rename this to pushedFilters
?
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.
pushedFilters
conflicts with Spark method name, rename it to pushedPredicates
} | ||
|
||
// Only support with push down partition filter | ||
if (pushed.length != partitionFilter.length) { |
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.
Can we also save postScanFilters
in pushFilters
, and just check is empty 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.
+1, added postScanFilters
} | ||
|
||
val readBuilder = table.newReadBuilder | ||
if (pushed.nonEmpty) { |
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.
just use partitionFilter?
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.
partitionFilters
is the Spark filter, but we need Paimon's predicate..
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.
@ulysses-you Thanks for your contribution!
Left some comments about naming.
+1 |
Link to #2404. |
Purpose
Make
PaimonScanBuilder
supportSupportsPushDownAggregates
. This pr implements a local aggregator framework to eval aggregate function. For now, onlyCountStar
is supported.If the aggregation can be pushed down, build a
PaimonLocalScan
to avoid execute with RDD.There are some limitations:
Tests
add test
API and Format
no
Documentation