Skip to content

[SPARK-3394] [SQL] Fix crash in TakeOrdered when limit is 0 #2264

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[SPARK-3394] [SQL] Fix crash in TakeOrdered when limit is 0 #2264

wants to merge 3 commits into from

Conversation

@ericl
Copy link
Contributor

@ericl ericl commented Sep 4, 2014

@AmplabJenkins
Copy link

AmplabJenkins commented Sep 4, 2014

Can one of the admins verify this patch?

@ericl ericl changed the title [SPARK-3394] Fix crash in TakeOrdered when limit is 0 [SPARK-3394] [SQL] Fix crash in TakeOrdered when limit is 0 Sep 4, 2014
@marmbrus
Copy link
Contributor

marmbrus commented Sep 4, 2014

ok to test

@SparkQA
Copy link

SparkQA commented Sep 4, 2014

QA tests have started for PR 2264 at commit 7a51528.

  • This patch merges cleanly.

@liancheng
Copy link
Contributor

liancheng commented Sep 4, 2014

I'm thinking that maybe we should fix this issue by allowing RDD.takeOrdered accept 0 as argument?

@SparkQA
Copy link

SparkQA commented Sep 4, 2014

QA tests have finished for PR 2264 at commit 7a51528.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class BlockManagerMaster(
    • class AttributeMap[A](baseMap: Map[ExprId, (Attribute, A)])

@ericl
Copy link
Contributor Author

ericl commented Sep 4, 2014

That's a good point - and it seems the pyspark api for example does support takeOrdered(0). Updated.

@liancheng
Copy link
Contributor

liancheng commented Sep 4, 2014

Build failure caused by GraphX suite, I believe @ankurdave is looking into that. Let's wait for now.

aarondav
aarondav reviewed Sep 4, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/rdd/RDD.scala Outdated
Copy link
Contributor

@aarondav aarondav Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very minor, but Array.empty would be slightly more canonical.

Copy link
Contributor

@liancheng liancheng Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And maybe add an else to avoid the return :)

@ankurdave
Copy link
Contributor

ankurdave commented Sep 4, 2014

retest this please

I think it's a transient failure in GraphX?

@ericl
Copy link
Contributor Author

ericl commented Sep 4, 2014

addressed comments

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 4, 2014

Jenkins, retest this please.

@liancheng
Copy link
Contributor

liancheng commented Sep 4, 2014

retest this please.

Jenkins was down a couple hours ago and just came alive.

@rxin
Copy link
Contributor

rxin commented Sep 6, 2014

Jenkins, retest this please.

@rxin
Copy link
Contributor

rxin commented Sep 6, 2014

LGTM.

@SparkQA
Copy link

SparkQA commented Sep 6, 2014

QA tests have started for PR 2264 at commit c87355b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 6, 2014

QA tests have finished for PR 2264 at commit c87355b.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mateiz
Copy link
Contributor

mateiz commented Sep 7, 2014

Jenkins, test this please

@mateiz
Copy link
Contributor

mateiz commented Sep 7, 2014

(I believe that test is currently flaky)

@SparkQA
Copy link

SparkQA commented Sep 7, 2014

QA tests have started for PR 2264 at commit c87355b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 7, 2014

QA tests have finished for PR 2264 at commit c87355b.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Sep 7, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 7, 2014

QA tests have started for PR 2264 at commit c87355b.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 7, 2014

QA tests have finished for PR 2264 at commit c87355b.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mateiz
Copy link
Contributor

mateiz commented Sep 8, 2014

Seems like MIMA was confused in the previous run, but tests actually passed, so I'm going to merge this.

asfgit pushed a commit that referenced this pull request Sep 8, 2014
@mateiz
[SPARK-3394] [SQL] Fix crash in TakeOrdered when limit is 0
c5d8d82 
This resolves https://issues.apache.org/jira/browse/SPARK-3394

Author: Eric Liang <[email protected]>

Closes #2264 from ericl/spark-3394 and squashes the following commits:

c87355b [Eric Liang] refactor
bfb6140 [Eric Liang] change RDD takeOrdered instead
7a51528 [Eric Liang] fix takeordered when limit = 0

(cherry picked from commit 6754570)
Signed-off-by: Matei Zaharia <[email protected]>
asfgit pushed a commit that referenced this pull request Sep 8, 2014
@mateiz
[SPARK-3394] [SQL] Fix crash in TakeOrdered when limit is 0
ae6f554 
This resolves https://issues.apache.org/jira/browse/SPARK-3394

Author: Eric Liang <[email protected]>

Closes #2264 from ericl/spark-3394 and squashes the following commits:

c87355b [Eric Liang] refactor
bfb6140 [Eric Liang] change RDD takeOrdered instead
7a51528 [Eric Liang] fix takeordered when limit = 0

(cherry picked from commit 6754570)
Signed-off-by: Matei Zaharia <[email protected]>
@mateiz
Copy link
Contributor

mateiz commented Sep 8, 2014

Merged into 1.2, branch-1.1 and branch-1.0

@asfgit asfgit closed this in 6754570 Sep 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@ericl @AmplabJenkins @marmbrus @SparkQA @liancheng @ankurdave @JoshRosen @rxin @mateiz @aarondav