Skip to content
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.

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

SPARK-1456 Remove view bounds on Ordered in favor of a context bound on Ordering. #410

Closed
wants to merge 4 commits into from

Conversation

marmbrus
Copy link
Contributor

This doesn't require creating new Ordering objects per row. Additionally, view bounds are going to be deprecated, so we should get rid of them while APIs are still flexible.

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@@ -89,12 +89,14 @@ class HashPartitioner(partitions: Int) extends Partitioner {
* A [[org.apache.spark.Partitioner]] that partitions sortable records by range into roughly
* equal ranges. The ranges are determined by sampling the content of the RDD passed in.
*/
class RangePartitioner[K <% Ordered[K]: ClassTag, V](
class RangePartitioner[K : Ordering : ClassTag, V](
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the user still pass in an Ordering if they want a specific Ordering?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when de-sugared there is an implicit Ordering -- that's why Michael can recover and bind it explicitly at line 98. And yes, it can be overridden in the normal way.

@AmplabJenkins
Copy link

Merged build finished. All automated tests passed.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14135/

@@ -27,12 +27,14 @@ import org.apache.spark.{Logging, RangePartitioner}
* use these functions. They will work with any key type that has a `scala.math.Ordered`
* implementation.
*/
class OrderedRDDFunctions[K <% Ordered[K]: ClassTag,
class OrderedRDDFunctions[K : Ordering : ClassTag,
Copy link
Contributor

Choose a reason for hiding this comment

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

@marmbrus Do you mind updating the scaladoc above since now users can pass their own orderings in addition to an Ordered type? It might be nice to just give a one-line example of how they can define a custom ordering as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@AmplabJenkins
Copy link

Merged build finished. All automated tests passed.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14188/

* }
*
* // Sort by key, using the above case insensitive ordering.
* rdd.sortByKey
Copy link
Contributor

Choose a reason for hiding this comment

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

a nit: put parenthesis around sortByKey

@rxin
Copy link
Contributor

rxin commented Apr 17, 2014

We should also update the Java API to create Ordering directly (instead of Ordered). It still compiles now, but kind of strange to have Ordered in the Java API that gets converted into Ordering.

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@AmplabJenkins
Copy link

Merged build finished. All automated tests passed.

@AmplabJenkins
Copy link

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14240/

@rxin
Copy link
Contributor

rxin commented Apr 18, 2014

lgtm

@rxin
Copy link
Contributor

rxin commented Apr 18, 2014

merged.

@asfgit asfgit closed this in c399baa Apr 18, 2014
asfgit pushed a commit that referenced this pull request Apr 18, 2014
…on Ordering.

This doesn't require creating new Ordering objects per row.  Additionally, [view bounds are going to be deprecated](https://issues.scala-lang.org/browse/SI-7629), so we should get rid of them while APIs are still flexible.

Author: Michael Armbrust <michael@databricks.com>

Closes #410 from marmbrus/viewBounds and squashes the following commits:

c574221 [Michael Armbrust] fix example.
812008e [Michael Armbrust] Update Java API.
1b9b85c [Michael Armbrust] Update scala doc.
35798a8 [Michael Armbrust] Remove view bounds on Ordered in favor of a context bound on Ordering.

(cherry picked from commit c399baa)
Signed-off-by: Reynold Xin <rxin@apache.org>
@marmbrus marmbrus deleted the viewBounds branch April 23, 2014 22:01
pwendell pushed a commit to pwendell/spark that referenced this pull request May 12, 2014
Updated JavaStreamingContext to make scaladoc compile.

`sbt/sbt doc` used to fail. This fixed it.
pdeyhim pushed a commit to pdeyhim/spark-1 that referenced this pull request Jun 25, 2014
…on Ordering.

This doesn't require creating new Ordering objects per row.  Additionally, [view bounds are going to be deprecated](https://issues.scala-lang.org/browse/SI-7629), so we should get rid of them while APIs are still flexible.

Author: Michael Armbrust <michael@databricks.com>

Closes apache#410 from marmbrus/viewBounds and squashes the following commits:

c574221 [Michael Armbrust] fix example.
812008e [Michael Armbrust] Update Java API.
1b9b85c [Michael Armbrust] Update scala doc.
35798a8 [Michael Armbrust] Remove view bounds on Ordered in favor of a context bound on Ordering.
mccheah pushed a commit to mccheah/spark that referenced this pull request Nov 28, 2018
bzhaoopenstack pushed a commit to bzhaoopenstack/spark that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants