[SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter #7129

ilganeli · 2015-06-30T14:18:38Z

I've started by separating ExternalAggregator into two classes, one which assumes an aggregator is defined and one which does not. Common code is extracted into a parent class.

…class instances, one which uses Aggregator and one which does not. Next step is to extract out common code

…lasses that implement this interface to abstract away the aggregator usage.

SparkQA · 2015-06-30T16:29:44Z

Test build #36151 has finished for PR 7129 at commit b74ca30.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int, data: BufferedIterator[((Int, K), C)])

SparkQA · 2015-06-30T16:31:27Z

Test build #36139 has finished for PR 7129 at commit d0024ef.

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

SparkQA · 2015-06-30T16:35:21Z

Test build #36153 has finished for PR 7129 at commit 864d603.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int, data: BufferedIterator[((Int, K), C)])

SparkQA · 2015-06-30T16:41:28Z

Test build #36154 has finished for PR 7129 at commit 8b3aca5.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

…gregator interfaces

SparkQA · 2015-06-30T17:20:53Z

Test build #36163 has finished for PR 7129 at commit 72b87e5.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

ilganeli · 2015-06-30T17:48:15Z

retest this please

SparkQA · 2015-06-30T18:06:48Z

Test build #36168 has finished for PR 7129 at commit 083b2b6.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

SparkQA · 2015-06-30T21:17:42Z

Test build #36174 timed out for PR 7129 at commit 36db0dc after a configured wait of 175m.

SparkQA · 2015-07-01T14:30:15Z

Test build #36253 has finished for PR 7129 at commit 1a8dba5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

ilganeli · 2015-07-01T18:47:48Z

@JoshRosen Any chance I could get a second pair of eyes on this? I'm wary of changes to ExternalSorter introducing complicated merge conflicts. Would love your help, thanks!

SparkQA · 2015-07-05T17:46:49Z

Test build #36548 has finished for PR 7129 at commit 7ecc28d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

SparkQA · 2015-07-08T22:02:47Z

Test build #36842 has finished for PR 7129 at commit 12c11a7.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

SparkQA · 2015-07-08T23:02:07Z

Test build #36852 has finished for PR 7129 at commit 1c154c4.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

SparkQA · 2015-07-09T01:19:20Z

Test build #36855 has finished for PR 7129 at commit 5e5d5e7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

ilganeli · 2015-07-09T01:25:26Z

retest this please

SparkQA · 2015-07-09T03:57:29Z

Test build #36872 has finished for PR 7129 at commit 8f6e327.

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

ilganeli · 2015-07-09T04:12:54Z

retest this please

SparkQA · 2015-07-09T06:26:04Z

Test build #36888 has finished for PR 7129 at commit 8f6e327.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

ilganeli · 2015-07-10T17:12:56Z

@rxin @JoshRosen Could I please get a review of this PR? Thanks!

ilganeli · 2015-07-16T19:52:39Z

@JoshRosen @rxin Hi folks - any chance of getting a review? Thanks!

SparkQA · 2015-07-16T20:10:34Z

Test build #37525 has finished for PR 7129 at commit e5768fa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,

SparkQA · 2015-07-23T22:19:59Z

Test build #38259 has finished for PR 7129 at commit 1eca0e1.

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

ilganeli · 2015-07-31T16:12:05Z

@rxin @JoshRosen @davies Could I please get a review for this patch? Thanks.

JoshRosen · 2015-07-31T16:38:49Z

Realistically, I think it's unlikely that any of us will have time to review this until next week since we're all busy working on things for the 1.5.0 feature freeze.

SparkQA · 2015-07-31T18:52:47Z

Test build #39232 has finished for PR 7129 at commit bf63fce.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- protected[this] case class SpilledFile(
- protected[this] class SpillReader(spill: SpilledFile)
- protected[this] class IteratorForPartition(partitionId: Int,
- class MulticlassClassificationEvaluator (override val uid: String)
- class NaiveBayes(JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredictionCol):
- class NaiveBayesModel(JavaModel):
- class MulticlassClassificationEvaluator(JavaEvaluator, HasLabelCol, HasPredictionCol):
- abstract class TernaryExpression extends Expression

marmbrus · 2015-09-03T21:18:03Z

@ilganeli thanks a lot for working on this, but we've decided this probably isn't the right thing to do. Do you mind if we close this issue?

ilganeli · 2015-09-03T21:19:43Z

That’s fine, go for it.

From: Michael Armbrust <notifications@github.com mailto:notifications@github.com>
Reply-To: apache/spark <reply@reply.github.com mailto:reply@reply.github.com>
Date: Thursday, September 3, 2015 at 2:18 PM
To: apache/spark <spark@noreply.github.com mailto:spark@noreply.github.com>
Cc: "Ganelin, Ilya" <ilya.ganelin@capitalone.com mailto:ilya.ganelin@capitalone.com>
Subject: Re: [spark] [SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter (#7129)

@ilganelihttps://github.com/ilganeli thanks a lot for working on this, but we've decided this probably isn't the right thing to do. Do you mind if we close this issue?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/7129#issuecomment-137577700.

The information contained in this e-mail is confidential and/or proprietary to Capital One and/or its affiliates and may only be used solely in performance of work or services for Capital One. The information transmitted herewith is intended only for use by the individual or entity to which it is addressed. If the reader of this message is not the intended recipient, you are hereby notified that any review, retransmission, dissemination, distribution, copying or other use of, or taking of any action in reliance upon this information is strictly prohibited. If you have received this communication in error, please contact the sender and delete the material from your computer.

Ilya Ganelin added 2 commits June 30, 2015 10:14

[SPARK-8464] As a starting point, refactored ExternalSorter into two …

d0024ef

…class instances, one which uses Aggregator and one which does not. Next step is to extract out common code

Created a top level interface for ExternalSorter and created two subc…

4a04b2a

…lasses that implement this interface to abstract away the aggregator usage.

ilganeli changed the title ~~[SPARK-8464][Core][Shuffle][WIP] Consider separating aggregator and non-aggregator paths in ExternalSorter~~ [SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter Jun 30, 2015

Ilya Ganelin added 2 commits June 30, 2015 12:22

Compilation fixes

b74ca30

Spacing

864d603

Style fixes

8b3aca5

Updated creation of ExternalSorter instances to use aggregator//no-ag…

72b87e5

…gregator interfaces

Style

083b2b6

Added MIMA exclude

36db0dc

Fixed error in constructor

1a8dba5

Fixing merge conflicts

7ecc28d

Ilya Ganelin added 2 commits July 8, 2015 14:54

Fixes to MimaExcludes

61a4c8e

Resolved merge conflict

12c11a7

style fix

1c154c4

Fixing merge conflict

5e5d5e7

Merge conflict

8f6e327

Resolved merge conflicts

e5768fa

Fixed merge conflict

1eca0e1

Ilya Ganelin added 3 commits July 31, 2015 09:08

Moved sort updates from basicOperator.scala to sort.scala

71eef54

Import ordering and spacing/

d2fe6b0

Spacing

bf63fce

asfgit closed this in 804a012 Sep 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter #7129

[SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter #7129

ilganeli commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

ilganeli commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jul 1, 2015

ilganeli commented Jul 1, 2015

SparkQA commented Jul 5, 2015

SparkQA commented Jul 8, 2015

SparkQA commented Jul 8, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 9, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 9, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 10, 2015

ilganeli commented Jul 16, 2015

SparkQA commented Jul 16, 2015

SparkQA commented Jul 23, 2015

ilganeli commented Jul 31, 2015

JoshRosen commented Jul 31, 2015

SparkQA commented Jul 31, 2015

marmbrus commented Sep 3, 2015

ilganeli commented Sep 3, 2015

[SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter #7129

[SPARK-8464][Core][Shuffle] Consider separating aggregator and non-aggregator paths in ExternalSorter #7129

Conversation

ilganeli commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

ilganeli commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jun 30, 2015

SparkQA commented Jul 1, 2015

ilganeli commented Jul 1, 2015

SparkQA commented Jul 5, 2015

SparkQA commented Jul 8, 2015

SparkQA commented Jul 8, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 9, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 9, 2015

SparkQA commented Jul 9, 2015

ilganeli commented Jul 10, 2015

ilganeli commented Jul 16, 2015

SparkQA commented Jul 16, 2015

SparkQA commented Jul 23, 2015

ilganeli commented Jul 31, 2015

JoshRosen commented Jul 31, 2015

SparkQA commented Jul 31, 2015

marmbrus commented Sep 3, 2015

ilganeli commented Sep 3, 2015