Skip to content

Fixed a typo in the comments in RangePartitioner#1473

Closed
dorx wants to merge 2 commits intoapache:masterfrom
dorx:typoInPartitioner
Closed

Fixed a typo in the comments in RangePartitioner#1473
dorx wants to merge 2 commits intoapache:masterfrom
dorx:typoInPartitioner

Conversation

@dorx
Copy link
Contributor

@dorx dorx commented Jul 18, 2014

Checked with Holden, the original author as per the log, and was told
code is right comment is wrong.

Checked with Holden, the original author as per the log, and was told
code is right comment is wrong.
@SparkQA
Copy link

SparkQA commented Jul 18, 2014

QA tests have started for PR 1473. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16796/consoleFull

@SparkQA
Copy link

SparkQA commented Jul 18, 2014

QA results for PR 1473:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16796/consoleFull

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually 1000 seems a pretty large number for doing linear scan. How about 64 or 128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I noticed it in the first place. Would changing this number have unintended affects on people who're currently using the RangePartitioner?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. If anything, it should make it faster.

@SparkQA
Copy link

SparkQA commented Jul 18, 2014

QA tests have started for PR 1473. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16816/consoleFull

@SparkQA
Copy link

SparkQA commented Jul 18, 2014

QA results for PR 1473:
- This patch FAILED unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16816/consoleFull

@dorx
Copy link
Contributor Author

dorx commented Jul 18, 2014

A superficial look at the failed unit tests seems to suggest some Spark SQL optimizations rely on the fact that 1000 is set as the sequential scan threshhold. @rxin @marmbrus

@marmbrus
Copy link
Contributor

It appears to me that the range partitioner is not correctly using the provided ordering in the case where it uses a binary search.

@rxin
Copy link
Contributor

rxin commented Jul 20, 2014

I filed a JIRA: https://issues.apache.org/jira/browse/SPARK-2598

@rxin
Copy link
Contributor

rxin commented Jul 20, 2014

@dorx can you close this PR? #1500 includes the change here.

@dorx dorx closed this Jul 20, 2014
@dorx dorx deleted the typoInPartitioner branch July 20, 2014 19:13
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.

4 participants