[SPARK-32776][SS] Limit in streaming should not be optimized away by PropagateEmptyRelation #29623
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @HeartSaVioR |
HeartSaVioR
approved these changes
Sep 2, 2020
There was a problem hiding this comment.
LGTM assuming the new test would fail in master branch.
Ideally the fundamental fix is to ensure state stores to write delta files for all state stores in the query per batch, instead of caring all of stateful operations in complicated rules. That sounds a bit hard to achieve, so let's see who brings a good idea to deal with.
|
Btw, looks like we are missing to update the guide doc once we address limitation. According to the guide, limit is not supported in streaming dataset. |
|
Test build #128174 has finished for PR 29623 at commit
|
HyukjinKwon
reviewed
Sep 2, 2020
| @@ -257,4 +257,10 @@ class PropagateEmptyRelationSuite extends PlanTest { | |||
| val optimized = Optimize.execute(query.analyze) | |||
| assert(optimized.resolved) | |||
| } | |||
|
|
|||
| test("should not optimize away limit if streaming") { | |||
There was a problem hiding this comment.
shall we add a JIRA prefix?
Suggested change
| test("should not optimize away limit if streaming") { | |
| test("SPARK-32776: should not optimize away limit if streaming") { |
HyukjinKwon
reviewed
Sep 2, 2020
| @@ -1141,6 +1143,42 @@ class StreamingQuerySuite extends StreamTest with BeforeAndAfter with Logging wi | |||
| } | |||
| } | |||
|
|
|||
| testQuietly("limit on empty batch should not cause state store error") { | |||
There was a problem hiding this comment.
Suggested change
| testQuietly("limit on empty batch should not cause state store error") { | |
| testQuietly("SPARK-32776: limit on empty batch should not cause state store error") { |
HyukjinKwon
changed the title
|
I am just going to merge since the PR is ready to go. |
|
Merged to master and branch-3.0. |
HyukjinKwon
pushed a commit
that referenced
this pull request
Sep 2, 2020
…PropagateEmptyRelation PropagateEmptyRelation will not be applied to LIMIT operators in streaming queries. Right now, the limit operator in a streaming query may get optimized away when the relation is empty. This can be problematic for stateful streaming, as this empty batch will not write any state store files, and the next batch will fail when trying to read these state store files and throw a file not found error. We should not let PropagateEmptyRelation optimize away the Limit operator for streaming queries. This PR is intended as a small and safe fix for PropagateEmptyRelation. A fundamental fix that can prevent this from happening again in the future and in other optimizer rules is more desirable, but that's a much larger task. No unit tests. Closes #29623 from liwensun/spark-32776. Authored-by: liwensun <liwen.sun@databricks.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit f0851e9) Signed-off-by: HyukjinKwon <gurwls223@apache.org>
holdenk
pushed a commit
to holdenk/spark
that referenced
this pull request
Oct 27, 2020
…PropagateEmptyRelation PropagateEmptyRelation will not be applied to LIMIT operators in streaming queries. Right now, the limit operator in a streaming query may get optimized away when the relation is empty. This can be problematic for stateful streaming, as this empty batch will not write any state store files, and the next batch will fail when trying to read these state store files and throw a file not found error. We should not let PropagateEmptyRelation optimize away the Limit operator for streaming queries. This PR is intended as a small and safe fix for PropagateEmptyRelation. A fundamental fix that can prevent this from happening again in the future and in other optimizer rules is more desirable, but that's a much larger task. No unit tests. Closes apache#29623 from liwensun/spark-32776. Authored-by: liwensun <liwen.sun@databricks.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit f0851e9) Signed-off-by: HyukjinKwon <gurwls223@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
PropagateEmptyRelation will not be applied to LIMIT operators in streaming queries.
Why are the changes needed?
Right now, the limit operator in a streaming query may get optimized away when the relation is empty. This can be problematic for stateful streaming, as this empty batch will not write any state store files, and the next batch will fail when trying to read these state store files and throw a file not found error.
We should not let PropagateEmptyRelation optimize away the Limit operator for streaming queries.
This PR is intended as a small and safe fix for PropagateEmptyRelation. A fundamental fix that can prevent this from happening again in the future and in other optimizer rules is more desirable, but that's a much larger task.
Does this PR introduce any user-facing change?
No
How was this patch tested?
unit tests.