SPARK-1438 RDD make seed optional in RDD methods sam... #462

arun-rama · 2014-04-21T07:57:16Z

Its probably better to let the underlying language implementation take care of the default seed if none is specified by the user. This was easier to do with python as the default value for seed in random and numpy random is None.

In Scala/Java side it might meen propagating an Option or null(oh no!) down the chain until where the Random is constructed. But, looks like the convention in some other methods was to use System.nanoTime. So, followed that convention.

Conflict with overloaded method in sql.SchemaRDD
SchemaRDD defines an overloaded method
sample(fraction, withReplacement=false, seed=math.random)

So, SchemaRDD had tow sample methods with same parameters in different order. I believe the author intended to override the RDD.sample method and not overload it. So, changed it.

Also, scala does not allow more than overloaded method to have default params. So, this code had to be modified. Not sure if there is exiting application code that might break because of this. If we need to keep things backward compatible, 3 new method can be introduced (without default params) like this
sample(fraction)
sample(fraction, withReplacement)
sample(fraction, withReplacement, seed)

Added some tests for the scala RDD takeSample method. Was able to test the java side manually.

…sample/takeSample

AmplabJenkins · 2014-04-21T07:58:12Z

Can one of the admins verify this patch?

advancedxy · 2014-04-21T11:40:59Z

core/src/main/scala/org/apache/spark/api/java/JavaPairRDD.scala

+  /**
+   * Return a sampled subset of this RDD.
+   */
+  def sample(withReplacement: Boolean, fraction: Double, seed: Long): JavaPairRDD[K, V] =


I think it's import to use Int instead of Long. Since current code is wrote against Int. If we change to Long, the old code using the sample api cannot be compiled because of type mismatch.

Long is going to be more standard; certainly Java uses long seeds in its APIs. It gives more bits of seed, which is good too. There may be some API changes but anyone calling with an Int seed should be able to call an API with a Long seed right?

Yes. It is more standard to use Long. But we just need to rewrite some code.
We should ask about @mateiz or @pwendell for advice. Maybe they chosen Int for some reason we don't know.

We can have deprecated overloaded methods for backward compatibility if needed.

advancedxy · 2014-04-21T11:45:57Z

About the seed selection, maybe we should keep all the implementations in sync.
That is to say: if we use system.nanotime in scala, we should use it in java and python; if we use math.random, we should use it in java and python(or port it to different languages).

aarondav · 2014-04-21T16:20:12Z

sql/core/src/main/scala/org/apache/spark/sql/SchemaRDD.scala

      withReplacement: Boolean = true,
-      seed: Int = (math.random * 1000).toInt) =
+      fraction: Double,
+      seed: Long) =


Did you intend to remove the default behavior here?

Scala does not allow multiple overloaded methods to have default params. So, if we makde seed default in RDD.sample, then this had to be modified. So, modified it in a standard way. Also, I believe the author intended to overrride RDD.sample and not overload. More details on the PR comment.

pwendell · 2014-04-21T17:29:08Z

Could you make this pull request into the master branch instead of into branch-1.0? Thanks

arun-rama · 2014-04-22T05:34:57Z

new PR against master instead of 1.0 #477

copying form previous pull request #462 Its probably better to let the underlying language implementation take care of the default . This was easier to do with python as the default value for seed in random and numpy random is None. In Scala/Java side it might mean propagating an Option or null(oh no!) down the chain until where the Random is constructed. But, looks like the convention in some other methods was to use System.nanoTime. So, followed that convention. Conflict with overloaded method in sql.SchemaRDD.sample which also defines default params. sample(fraction, withReplacement=false, seed=math.random) Scala does not allow more than one overloaded to have default params. I believe the author intended to override the RDD.sample method and not overload it. So, changed it. If backward compatible is important, 3 new method can be introduced (without default params) like this sample(fraction) sample(fraction, withReplacement) sample(fraction, withReplacement, seed) Added some tests for the scala RDD takeSample method. Author: Arun Ramakrishnan <[email protected]> This patch had conflicts when merged, resolved by Committer: Matei Zaharia <[email protected]> Closes #477 from smartnut007/master and squashes the following commits: 07bb06e [Arun Ramakrishnan] SPARK-1438 fixing more space formatting issues b9ebfe2 [Arun Ramakrishnan] SPARK-1438 removing redundant import of random in python rddsampler 8d05b1a [Arun Ramakrishnan] SPARK-1438 RDD . Replace System.nanoTime with a Random generated number. python: use a separate instance of Random instead of seeding language api global Random instance. 69619c6 [Arun Ramakrishnan] SPARK-1438 fix spacing issue 0c247db [Arun Ramakrishnan] SPARK-1438 RDD language apis to support optional seed in RDD methods sample/takeSample (cherry picked from commit 35e3d19) Signed-off-by: Matei Zaharia <[email protected]>

copying form previous pull request #462 Its probably better to let the underlying language implementation take care of the default . This was easier to do with python as the default value for seed in random and numpy random is None. In Scala/Java side it might mean propagating an Option or null(oh no!) down the chain until where the Random is constructed. But, looks like the convention in some other methods was to use System.nanoTime. So, followed that convention. Conflict with overloaded method in sql.SchemaRDD.sample which also defines default params. sample(fraction, withReplacement=false, seed=math.random) Scala does not allow more than one overloaded to have default params. I believe the author intended to override the RDD.sample method and not overload it. So, changed it. If backward compatible is important, 3 new method can be introduced (without default params) like this sample(fraction) sample(fraction, withReplacement) sample(fraction, withReplacement, seed) Added some tests for the scala RDD takeSample method. Author: Arun Ramakrishnan <[email protected]> This patch had conflicts when merged, resolved by Committer: Matei Zaharia <[email protected]> Closes #477 from smartnut007/master and squashes the following commits: 07bb06e [Arun Ramakrishnan] SPARK-1438 fixing more space formatting issues b9ebfe2 [Arun Ramakrishnan] SPARK-1438 removing redundant import of random in python rddsampler 8d05b1a [Arun Ramakrishnan] SPARK-1438 RDD . Replace System.nanoTime with a Random generated number. python: use a separate instance of Random instead of seeding language api global Random instance. 69619c6 [Arun Ramakrishnan] SPARK-1438 fix spacing issue 0c247db [Arun Ramakrishnan] SPARK-1438 RDD language apis to support optional seed in RDD methods sample/takeSample

Remove Typesafe Config usage and conf files to fix nested property names With Typesafe Config we had the subtle problem of no longer allowing nested property names, which are used for a few of our properties: http://apache-spark-developers-list.1001551.n3.nabble.com/Config-properties-broken-in-master-td208.html This PR is for branch 0.9 but should be added into master too. (cherry picked from commit 34e911c) Signed-off-by: Patrick Wendell <[email protected]>

copying form previous pull request apache#462 Its probably better to let the underlying language implementation take care of the default . This was easier to do with python as the default value for seed in random and numpy random is None. In Scala/Java side it might mean propagating an Option or null(oh no!) down the chain until where the Random is constructed. But, looks like the convention in some other methods was to use System.nanoTime. So, followed that convention. Conflict with overloaded method in sql.SchemaRDD.sample which also defines default params. sample(fraction, withReplacement=false, seed=math.random) Scala does not allow more than one overloaded to have default params. I believe the author intended to override the RDD.sample method and not overload it. So, changed it. If backward compatible is important, 3 new method can be introduced (without default params) like this sample(fraction) sample(fraction, withReplacement) sample(fraction, withReplacement, seed) Added some tests for the scala RDD takeSample method. Author: Arun Ramakrishnan <[email protected]> This patch had conflicts when merged, resolved by Committer: Matei Zaharia <[email protected]> Closes apache#477 from smartnut007/master and squashes the following commits: 07bb06e [Arun Ramakrishnan] SPARK-1438 fixing more space formatting issues b9ebfe2 [Arun Ramakrishnan] SPARK-1438 removing redundant import of random in python rddsampler 8d05b1a [Arun Ramakrishnan] SPARK-1438 RDD . Replace System.nanoTime with a Random generated number. python: use a separate instance of Random instead of seeding language api global Random instance. 69619c6 [Arun Ramakrishnan] SPARK-1438 fix spacing issue 0c247db [Arun Ramakrishnan] SPARK-1438 RDD language apis to support optional seed in RDD methods sample/takeSample

Remove Typesafe Config usage and conf files to fix nested property names With Typesafe Config we had the subtle problem of no longer allowing nested property names, which are used for a few of our properties: http://apache-spark-developers-list.1001551.n3.nabble.com/Config-properties-broken-in-master-td208.html This PR is for branch 0.9 but should be added into master too.

copying form previous pull request apache/spark#462 Its probably better to let the underlying language implementation take care of the default . This was easier to do with python as the default value for seed in random and numpy random is None. In Scala/Java side it might mean propagating an Option or null(oh no!) down the chain until where the Random is constructed. But, looks like the convention in some other methods was to use System.nanoTime. So, followed that convention. Conflict with overloaded method in sql.SchemaRDD.sample which also defines default params. sample(fraction, withReplacement=false, seed=math.random) Scala does not allow more than one overloaded to have default params. I believe the author intended to override the RDD.sample method and not overload it. So, changed it. If backward compatible is important, 3 new method can be introduced (without default params) like this sample(fraction) sample(fraction, withReplacement) sample(fraction, withReplacement, seed) Added some tests for the scala RDD takeSample method. Author: Arun Ramakrishnan <[email protected]> This patch had conflicts when merged, resolved by Committer: Matei Zaharia <[email protected]> Closes #477 from smartnut007/master and squashes the following commits: 07bb06e [Arun Ramakrishnan] SPARK-1438 fixing more space formatting issues b9ebfe2 [Arun Ramakrishnan] SPARK-1438 removing redundant import of random in python rddsampler 8d05b1a [Arun Ramakrishnan] SPARK-1438 RDD . Replace System.nanoTime with a Random generated number. python: use a separate instance of Random instead of seeding language api global Random instance. 69619c6 [Arun Ramakrishnan] SPARK-1438 fix spacing issue 0c247db [Arun Ramakrishnan] SPARK-1438 RDD language apis to support optional seed in RDD methods sample/takeSample

## Upstream SPARK-XXXXX ticket and PR link (if not applicable, explain) https://issues.apache.org/jira/browse/SPARK-26200 ## What changes were proposed in this pull request? Row type is handled differently depending on _needSerializeAnyField value. When _needSerializeAnyField, Row is handled as tuple which leads to column values being transposed (see upstream ticket for details). ## How was this patch tested? Unit test.

…esignate Remove enable_services of designate devstack conf

SPARK-1438 RDD language apis to support optional seed in RDD methods …

cb240b3

…sample/takeSample

advancedxy reviewed Apr 21, 2014
View reviewed changes

aarondav reviewed Apr 21, 2014
View reviewed changes

marmbrus mentioned this pull request Apr 21, 2014

[SPARK-1460] Returning SchemaRDD instead of normal RDD on Set operations... #448

Closed

arun-rama mentioned this pull request Apr 22, 2014

SPARK-1438 RDD.sample() make seed param optional #477

Closed

arun-rama closed this Apr 22, 2014

arun-rama deleted the branch-1.0 branch April 22, 2014 05:35

arun-rama restored the branch-1.0 branch April 22, 2014 05:38

markhamstra pushed a commit to markhamstra/spark that referenced this pull request Nov 7, 2017

Add command echoing for better command debugging (apache#462)

6177bf8

bzhaoopenstack pushed a commit to bzhaoopenstack/spark that referenced this pull request Sep 11, 2019

Merge pull request apache#462 from theopenlab/correct-devstack-conf-d…

2086c74

…esignate Remove enable_services of designate devstack conf

RolatZhang pushed a commit to RolatZhang/spark that referenced this pull request Aug 15, 2022

fix ut (apache#462)

af66839

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SPARK-1438 RDD make seed optional in RDD methods sam... #462

SPARK-1438 RDD make seed optional in RDD methods sam... #462

Uh oh!

arun-rama commented Apr 21, 2014

Uh oh!

AmplabJenkins commented Apr 21, 2014

Uh oh!

advancedxy Apr 21, 2014

Uh oh!

srowen Apr 21, 2014

Uh oh!

advancedxy Apr 21, 2014

Uh oh!

arun-rama Apr 21, 2014

Uh oh!

advancedxy commented Apr 21, 2014

Uh oh!

aarondav Apr 21, 2014

Uh oh!

arun-rama Apr 21, 2014

Uh oh!

pwendell commented Apr 21, 2014

Uh oh!

arun-rama commented Apr 22, 2014

Uh oh!

Uh oh!

SPARK-1438 RDD make seed optional in RDD methods sam... #462

SPARK-1438 RDD make seed optional in RDD methods sam... #462

Uh oh!

Conversation

arun-rama commented Apr 21, 2014

Uh oh!

AmplabJenkins commented Apr 21, 2014

Uh oh!

advancedxy Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

srowen Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

advancedxy Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

arun-rama Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Apr 21, 2014

Uh oh!

aarondav Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

arun-rama Apr 21, 2014

Choose a reason for hiding this comment

Uh oh!

pwendell commented Apr 21, 2014

Uh oh!

arun-rama commented Apr 22, 2014

Uh oh!

Uh oh!