-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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-22363][SQL][TEST] Add unit test for Window spilling #20022
Conversation
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.
Looks pretty good, also cc @gatorsmile @cloud-fan
test("SPARK-21258: complex object in combination with spilling") { | ||
// Make sure we trigger the spilling path. | ||
withSQLConf(SQLConf.WINDOW_EXEC_BUFFER_SPILL_THRESHOLD.key -> "17") { | ||
withSQLConf(SQLConf.WINDOW_EXEC_BUFFER_IN_MEMORY_THRESHOLD.key -> "0", |
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.
Why should we set this value to 0?
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.
WINDOW_EXEC_BUFFER_IN_MEMORY_THRESHOLD drives how much items is guaranteed to kept in memory. If this limit is not hit spilling not considered.
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.
Yeah, i mean, how about set it to 1 instead of 0?
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.
Ahh, now I see 🙂 Sure, I'll set it soon.
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.
We can accept any value. No limit. both are fine.
OK to test |
ok to test |
Test build #85540 has finished for PR 20022 at commit
|
retest this please |
Test build #85547 has finished for PR 20022 at commit
|
df.select($"key", sum("value").over(window)).collect() | ||
} | ||
} | ||
} |
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.
Normally, we will create a helper function for avoiding the duplicate codes. Since the test cases are pretty small, it is also fine.
LGTM |
Thanks! Merged to master. |
@gatorsmile @cloud-fan @jiangxb1987 Thanks for the help! |
What changes were proposed in this pull request?
There is already test using window spilling, but the test coverage is not ideal.
In this PR the already existing test was fixed and additional cases added.
How was this patch tested?
Automated: Pass the Jenkins.