[SPARK-13674][SQL] Add wholestage codegen support to Sample #11517

viirya · 2016-03-04T09:49:05Z

JIRA: https://issues.apache.org/jira/browse/SPARK-13674

What changes were proposed in this pull request?

Sample operator doesn't support wholestage codegen now. This pr is to add support to it.

How was this patch tested?

A test is added into BenchmarkWholeStageCodegen. Besides, all tests should be passed.

viirya · 2016-03-04T09:52:48Z

sql/core/src/test/scala/org/apache/spark/sql/execution/BenchmarkWholeStageCodegen.scala

+    range/sample withRep.:              Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
+    -------------------------------------------------------------------------------------------
+    range/sample withRep. codegen=false       149 /  238          0.3        2908.4       1.0X
+    range/sample withRep. codegen=true        192 /  206          0.3        3751.7       0.8X


The performance regression should be due to the current implementation of two samplers used in Sample operator. The samplers take iterator and return iterator. However, since we consume individual elements from parent operator in Sample, now this change needs to create another iterator from these elements.

I will try to implement another version of the two samplers without iterator. It should improve the performance here.

SparkQA · 2016-03-04T11:29:40Z

Test build #52460 has finished for PR 11517 at commit f14393a.

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

…mple

SparkQA · 2016-03-07T11:42:41Z

Test build #52546 has finished for PR 11517 at commit 7d50f5f.

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

…mple

viirya · 2016-03-09T06:04:53Z

core/src/main/scala/org/apache/spark/util/random/RandomSampler.scala

+   * Return how many times the next item will be sampled. Return 0 if it is not sampled.
+   */
+  def sample(): Int
+


These changes on RandomSampler is submitted in #11578.

If #11578 is merged, I will rebase this.

viirya · 2016-03-09T06:07:03Z

cc @davies @nongli

SparkQA · 2016-03-09T08:04:27Z

Test build #52726 has finished for PR 11517 at commit 6144810.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class GapSampling(
- trait PoissonGE
- class GapSamplingReplacementIterator[T: ClassTag](
- class GapSamplingReplacement(

…mple

SparkQA · 2016-03-21T09:44:44Z

Test build #53665 has finished for PR 11517 at commit 0cb714b.

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

nongli · 2016-03-21T14:31:56Z

core/src/main/scala/org/apache/spark/util/random/RandomSampler.scala


  override def setSeed(seed: Long): Unit = rng.setSeed(seed)

+  override def sample(): Int = {


Can we not have the code duplication and write the iterator version based on this function?

We can. As the discussion in #11578, I am just not sure if we want to remove the iterator-based implementation.

JIRA: https://issues.apache.org/jira/browse/SPARK-13742 ## What changes were proposed in this pull request? `RandomSampler.sample` currently accepts iterator as input and output another iterator. This makes it inappropriate to use in wholestage codegen of `Sampler` operator apache#11517. This change is to add non-iterator interface to `RandomSampler`. This change adds a new method `def sample(): Int` to the trait `RandomSampler`. As we don't need to know the actual values of the sampling items, so this new method takes no arguments. This method will decide whether to sample the next item or not. It returns how many times the next item will be sampled. For `BernoulliSampler` and `BernoulliCellSampler`, the returned sampling times can only be 0 or 1. It simply means whether to sample the next item or not. For `PoissonSampler`, the returned value can be more than 1, meaning the next item will be sampled multiple times. ## How was this patch tested? Tests are added into `RandomSamplerSuite`. Author: Liang-Chi Hsieh <simonh@tw.ibm.com> Author: Liang-Chi Hsieh <viirya@appier.com> Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#11578 from viirya/random-sampler-no-iterator.

…mple Conflicts: core/src/main/scala/org/apache/spark/util/random/RandomSampler.scala

SparkQA · 2016-03-29T05:21:43Z

Test build #54409 has finished for PR 11517 at commit 28073d5.

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

viirya · 2016-03-29T06:06:37Z

@davies This is rebased. Please take a look. Thanks.

davies · 2016-03-31T06:52:39Z

sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala

      |   $initTerm = true;
-      |   if ($input.hasNext()) {
-      |     initRange(((InternalRow) $input.next()).getInt(0));
+      |   if (partitionIndex != -1) {


davies · 2016-03-31T07:07:05Z

@viirya Could you also add numOutputRow for Sample ?

davies · 2016-03-31T07:08:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala

+      ctx.addMutableState(s"$samplerClass<UnsafeRow>", sampler,
+        s"$initSampler();")
+
+      val random = ctx.freshName("random")


Since these are used in a separate function, we don't need to generate fresh name for them.

SparkQA · 2016-03-31T10:21:44Z

Test build #54614 has finished for PR 11517 at commit fa51f62.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class PoissonSampler[T](

…mple Conflicts: project/MimaExcludes.scala

SparkQA · 2016-03-31T10:55:25Z

Test build #54618 has finished for PR 11517 at commit 76be6cf.

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

viirya · 2016-03-31T11:19:36Z

retest this please.

SparkQA · 2016-03-31T11:34:53Z

Test build #54620 has finished for PR 11517 at commit 76be6cf.

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

viirya · 2016-03-31T14:10:51Z

Not sure why the test failed..

viirya · 2016-03-31T14:10:58Z

retest this please.

SparkQA · 2016-03-31T16:08:40Z

Test build #54629 has finished for PR 11517 at commit 76be6cf.

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

…mple

…mple Conflicts: project/MimaExcludes.scala

SparkQA · 2016-04-01T07:20:25Z

Test build #54686 has finished for PR 11517 at commit ef588db.

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

davies · 2016-04-01T07:26:35Z

sql/core/src/test/scala/org/apache/spark/sql/execution/BenchmarkWholeStageCodegen.scala

+    */
+  }
+
+  ignore("sort merge join/sample") {


We may do not want this

ok. Let me remove it.

SparkQA · 2016-04-01T08:49:48Z

Test build #54693 has finished for PR 11517 at commit 12e1b37.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-04-01T08:53:13Z

retest this please.

SparkQA · 2016-04-01T09:28:25Z

Test build #54690 has finished for PR 11517 at commit 6dfecf1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class BatchPythonEvaluation(udfs: Seq[PythonUDF], output: Seq[Attribute], child: SparkPlan)
- // enable memo iff we serialize the row with schema (schema and class should be memorized)

SparkQA · 2016-04-01T10:59:49Z

Test build #54696 has finished for PR 11517 at commit 12e1b37.

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

davies · 2016-04-01T21:02:16Z

Chatted with @mengxr , it's OK to remove the class tag.

LGTM, merging this into master, thanks!

Init import for adding wholestage codegen support to Sample.

f14393a

viirya reviewed Mar 4, 2016
View reviewed changes

viirya added 2 commits March 7, 2016 17:08

Use Java list to avoid weird janino error.

38e94b4

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

7d50f5f

…mple

This was referenced Mar 7, 2016

[SPARK-13717][Core] Let RandomSampler can sample with Java iterator #11559

Closed

[SPARK-13742][Core] Add non-iterator interface to RandomSampler #11578

Closed

viirya added 3 commits March 9, 2016 11:16

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

e548585

…mple

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

f965975

…mple

Use new RandomSampler API to do wholestage codegen of Sampler.

6144810

viirya reviewed Mar 9, 2016
View reviewed changes

viirya added 2 commits March 21, 2016 06:16

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

4f8b327

…mple

With updated API.

0cb714b

nongli reviewed Mar 21, 2016
View reviewed changes

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

28073d5

…mple Conflicts: core/src/main/scala/org/apache/spark/util/random/RandomSampler.scala

davies reviewed Mar 31, 2016
View reviewed changes

Address comments. Mainly remove classtag.

fa51f62

viirya added 2 commits March 31, 2016 10:39

Fix MiMa and add numOutputRow for Sample.

d8a9af8

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

76be6cf

…mple Conflicts: project/MimaExcludes.scala

viirya added 3 commits April 1, 2016 02:39

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

e47e758

…mple

Consolidate two init APIs.

ef588db

Merge remote-tracking branch 'upstream/master' into add-wholestage-sa…

6dfecf1

…mple Conflicts: project/MimaExcludes.scala

davies reviewed Apr 1, 2016
View reviewed changes

Update tests.

12e1b37

asfgit closed this in 3e991db Apr 1, 2016

viirya deleted the add-wholestage-sample branch December 27, 2023 18:33


		override def setSeed(seed: Long): Unit = rng.setSeed(seed)

		override def sample(): Int = {

[SPARK-13674][SQL] Add wholestage codegen support to Sample #11517

[SPARK-13674][SQL] Add wholestage codegen support to Sample #11517

Uh oh!

Conversation

viirya commented Mar 4, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 4, 2016

Uh oh!

SparkQA commented Mar 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Mar 9, 2016

Uh oh!

SparkQA commented Mar 9, 2016

Uh oh!

SparkQA commented Mar 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 29, 2016

Uh oh!

viirya commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davies commented Mar 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

viirya commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

viirya commented Mar 31, 2016

Uh oh!

viirya commented Mar 31, 2016

Uh oh!

SparkQA commented Mar 31, 2016

Uh oh!

SparkQA commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 1, 2016

Uh oh!

viirya commented Apr 1, 2016

Uh oh!

SparkQA commented Apr 1, 2016

Uh oh!

SparkQA commented Apr 1, 2016

Uh oh!

davies commented Apr 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants