Support Aggregate push down for incremental scan #10538
Conversation
|
In this PR, I have basically extracted the code for building an |
|
cc @szehon-ho Could you please take a look when you have a moment? Thanks a lot! |
|
|
||
| assertThat(explain1).contains("LocalTableScan", "min(data)", "max(data)", "count(data)"); | ||
|
|
||
| Dataset<Row> noPushdownResult = |
There was a problem hiding this comment.
Naming nit: Could we suffix the Dataset variables with ds so this would be noPushdownDs and the other one would be pushdownDs?
There was a problem hiding this comment.
Also I don't think there's too much additional value in verifying the noPushdown case here. I think the test should just enable agg pushdown, and explicitly verify the expected results. But not super opinionated.
There was a problem hiding this comment.
Thanks @amogh-jahagirdar for the review! I have changed the variable names and removed the noPushdown cases.
There was a problem hiding this comment.
Thank you @huaxingao the change looks great to me. I'll wait in case @szehon-ho or others have comments before merging.
|
Merged, thanks @huaxingao and @amogh-jahagirdar for review, @RussellSpitzer for original review |
|
Thanks @amogh-jahagirdar, @szehon-ho, and of course, @RussellSpitzer |
|
I will have a follow-up PR to port the changes to Spark 3.4. @szehon-ho |
Enable Aggregate push down for incremental scan. Here is the original PR