Skip to content

Conversation

@apapi
Copy link
Contributor

@apapi apapi commented Jul 14, 2017

What changes were proposed in this pull request?

Fix a bug in RangePartitioner:
In RangePartitioner(partitions: Int, rdd: RDD[]), RangePartitioner.numPartitions is wrong if the number of elements in RDD (rdd.count()) is less than number of partitions (partitions in constructor).

How was this patch tested?

test as described in SPARK-SPARK-21410

Please review http://spark.apache.org/contributing.html before opening a pull request.

…the number of elements in RDD is less than number of partitions
@cloud-fan
Copy link
Contributor

Ok to test

@jiangxb1987
Copy link
Contributor

Please update the title to:

[SPARK-21410][CORE] Create less partitions for RangePartitioner if RDD.count() is less than `partitions`

@apapi apapi changed the title [SPARK-SPARK-21410][CORE] RangePartitioner.numPartitions is wrong if … [SPARK-21410][CORE] Create less partitions for RangePartitioner if RDD.count() is less than partitions Jul 17, 2017
@cloud-fan
Copy link
Contributor

ok to test

@cloud-fan
Copy link
Contributor

cc @srowen is it ok to do this? e.g. rdd.sortByKey(numPartitions = 10), do we have to return an RDD of 10 partitions even some of them are empty?

@SparkQA
Copy link

SparkQA commented Jul 17, 2017

Test build #79656 has finished for PR 18631 at commit 7239734.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Jul 17, 2017

That seems reasonable. An RDD can have empty partitions. The comment in the docs says that the number of partitions might be smaller than the requested number.

@jiangxb1987
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Jul 17, 2017

Test build #79662 has finished for PR 18631 at commit 7239734.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

@jiangxb1987 jiangxb1987 left a comment

Choose a reason for hiding this comment

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

LGTM

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@asfgit asfgit closed this in 7aac755 Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants