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-8638] [SQL] Window Function Performance Improvements #7057
Conversation
Jenkins, this is ok to test. |
Test build #35897 has finished for PR 7057 at commit
|
@hvanhovell Thank you for breaking the original PR to multiple ones. I am reviewing this one. Will add a comment once I finish my first round. |
* value of the order by clause and depends on its ordering. The group must be sorted for this to | ||
* produce sensible output. | ||
* - Shifted: The aggregate is a displaced value relative to the position of the given row. | ||
* Examples are Lead and Lag. |
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.
Seems we are mixing the concepts of (1) how a frame updates; and (2) how the frame boundary is determined together at here. Let me summarize them separately.
For frame boundary, we have two types, row and range.
For how frame updates, we have four types of frame:
- Entire partition: The frame is the entire partition, i.e.
UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
. For this case, window function will take all rows as inputs and be evaluated once. - Growing frame: We only add new rows into the frame, i.e.
UNBOUNDED PRECEDING AND ...
. Every time we move to a new row to process, we add some rows to the frame. We do not remove rows from this frame. - Shrinking frame: We only remove rows from the frame, i.e.
... AND UNBOUNDED FOLLOWING
. Every time we move to a new row to process, we remove some rows from the frame. We do not add rows to this frame. The frame will originally contain all rows of the partition. - Moving frame: Every time we move to a new row to process, we remove some rows from the frame and we add some rows to the frame. Examples are
1 PRECEDING AND CURRENT ROW
and1 FOLLOWING AND 2 FOLLOWING
.
I feel summarizing these two concepts separately can help people understand them. What do you think?
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.
I agree. There are still a few other documentation inconsistencies, and I'll try to fix those as well.
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.
For this PR, one of your main targets is to optimize Growing frame
, right? With your optimization, we can just update the aggregation buffer and get the evaluated results instead of creating a buffer for every row.
I see you also try to optimize Shrinking frame
by reversing the sort order. Then, we have to take care some functions very carefully (as you mentioned FIRST/LAST
). Also, since we reverse the sort order, the window function should be commutative. My concern is that if a user implement a user-defined window function that is commutative, he/she will not get the correct results. I feel the right way is before we add this optimization, we need to first have a separate task to add this kind properties to the function definition. Then, we optimize functions that are safe to optimize. For example, if a window function is commutative
(let's say the commutative
field in this function is true), we apply this optimization. Otherwise, we do not apply this optimization.
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.
The PR also optimizes the processing of Moving and Shrinking frames:
- For moving frame processing the number of comparisons are reduced. This didn't look like the most rewarding improvement, but I was surprised to find it did improved performance by quite a margin.
- Shrinking frames are indeed processed in reverse order. Which makes building it as fast as the growing case (it uses more memory though). I share your concerns, and solving this at the root (the function itself) would indeed be the best. I'll revert this for now, and file a JIRA request for future reference.
Test build #36225 timed out for PR 7057 at commit |
Test build #36267 has finished for PR 7057 at commit
|
* rows from this frame. | ||
* - Shrinking frame: We only remove rows from the frame, i.e. ... AND UNBOUNDED FOLLOWING. | ||
* Every time we move to a new row to process, we remove some rows from the frame. We do not add | ||
* rows to this frame. The frame will originally contain all rows of the partition. |
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.
I made a mistake at here. When we have n FOLLOWING AND UNBOUNDED FOLLOWING
, the frame will contain all rows of the partition at the beginning. So, we can remove The frame will originally contain all rows of the partition.
.
@hvanhovell I have finished my first round. Sorry for taking a long time. I think I understand the new workflow of the operator and it looks pretty good. I think it will be great if we can have more comments to explain how it works (specially for some important methods like |
@yhuai I have updated the PR. As for the documentation. I will add another section to the general class documentation, which explains the inner workings of the operator. Let me know what else needs some more documentation. |
Test build #36850 has finished for PR 7057 at commit
|
ok to test |
Test build #37244 has finished for PR 7057 at commit
|
(windowSpec.orderSpec, projection(), projection()) | ||
} | ||
// Use only the first order expression when the offset is non-null. | ||
else if (windowSpec.orderSpec.size == 1) { |
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.
How about we change the code format to something like the following?
if (...) {
// Comment to explain we reach here.
...
} else if () {
// Comment to explain we reach here.
...
} else {
// Comment to explain we reach here.
...
}
@hvanhovell I remember you have some benchmarking results. Can you add results to the description? Also, does your benchmark include tests for all of four kinds of frames (entire partition, growing frame, shrinking frame, and moving frame)? It will be good if we can have results for all these kinds of frames and we make sure there is no performance regression (I think it is unlikely that we introduce regression. But, it still good to have benchmarking results for different kinds of cases). |
Test build #37545 has finished for PR 7057 at commit
|
@yhuai the benchmarking results are attached. It might be interesting to see how the operator performs on different datasets. |
Test build #37567 has finished for PR 7057 at commit
|
…eorganization of code.
Test build #37743 has finished for PR 7057 at commit
|
@hvanhovell Overall looks good. I am merging it to master. I will leave a few comments for minor changes. Can you submit a follow-up PR to address them? |
* | ||
* TODO Move this class to the sql/core project when we move to Native Spark UDAFs. | ||
*/ | ||
class WindowSuite extends QueryTest { |
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.
Seems we do not need to create a new suite, right? We can just use HiveDataFrameWindowSuite
.
Description
Performance improvements for Spark Window functions. This PR will also serve as the basis for moving away from Hive UDAFs to Spark UDAFs. See JIRA tickets SPARK-8638 and SPARK-7712 for more information.
Improvements
Benchmarking
I have done a small benchmark using on time performance data of the month april. I have used the origin as a partioning key, as a result there is quite some variation in window sizes. The code for the benchmark can be found in the JIRA ticket. These are the results per Frame type: