[SPARK-17758][SQL] Last returns wrong result in case of empty partition #15348
Conversation
This file contains hidden or 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
|
Test build #66329 has finished for PR 15348 at commit
|
yhuai
reviewed
Oct 5, 2016
| // Update - Merge - Eval (empty partition at the end) | ||
| val m2 = evaluator.merge(p2, p1, p0) | ||
| assert(evaluator.eval(m2) === InternalRow(-99)) | ||
| } |
Contributor
There was a problem hiding this comment.
Have a test to call update using a null input and then check the answer?
Contributor
|
Thank you for fixing this! It is great to have unit tests to test individual aggregate functions. We can start to add more tests for other functions. |
|
Test build #66351 has finished for PR 15348 at commit
|
|
Test build #66349 has finished for PR 15348 at commit
|
Contributor
|
Spark does not preserve the order of partitions, how can we make sure the result is Row3, rather than Row2? |
Contributor
Author
|
You cannot. Last is not deterministic outside of ordered window functions. The only thing is that you can have inconsistent results if the last physical partition happens to be an empty one. |
Contributor
|
LGTM. Merging to master and branch 2.0. |
asfgit
pushed a commit
that referenced
this pull request
Oct 5, 2016
## What changes were proposed in this pull request? The result of the `Last` function can be wrong when the last partition processed is empty. It can return `null` instead of the expected value. For example, this can happen when we process partitions in the following order: ``` - Partition 1 [Row1, Row2] - Partition 2 [Row3] - Partition 3 [] ``` In this case the `Last` function will currently return a null, instead of the value of `Row3`. This PR fixes this by adding a `valueSet` flag to the `Last` function. ## How was this patch tested? We only used end to end tests for `DeclarativeAggregateFunction`s. I have added an evaluator for these functions so we can tests them in catalyst. I have added a `LastTestSuite` to test the `Last` aggregate function. Author: Herman van Hovell <hvanhovell@databricks.com> Closes #15348 from hvanhovell/SPARK-17758. (cherry picked from commit 5fd54b9) Signed-off-by: Yin Huai <yhuai@databricks.com>
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
## What changes were proposed in this pull request? The result of the `Last` function can be wrong when the last partition processed is empty. It can return `null` instead of the expected value. For example, this can happen when we process partitions in the following order: ``` - Partition 1 [Row1, Row2] - Partition 2 [Row3] - Partition 3 [] ``` In this case the `Last` function will currently return a null, instead of the value of `Row3`. This PR fixes this by adding a `valueSet` flag to the `Last` function. ## How was this patch tested? We only used end to end tests for `DeclarativeAggregateFunction`s. I have added an evaluator for these functions so we can tests them in catalyst. I have added a `LastTestSuite` to test the `Last` aggregate function. Author: Herman van Hovell <hvanhovell@databricks.com> Closes apache#15348 from hvanhovell/SPARK-17758.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
The result of the
Lastfunction can be wrong when the last partition processed is empty. It can returnnullinstead of the expected value. For example, this can happen when we process partitions in the following order:In this case the
Lastfunction will currently return a null, instead of the value ofRow3.This PR fixes this by adding a
valueSetflag to theLastfunction.How was this patch tested?
We only used end to end tests for
DeclarativeAggregateFunctions. I have added an evaluator for these functions so we can tests them in catalyst. I have added aLastTestSuiteto test theLastaggregate function.