Skip to content

[SPARK-14451][SQL] Move encoder definition into Aggregator interface #12231

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.

Sign up for GitHub

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

Jump to bottom
Closed
rxin wants to merge 8 commits into from
Closed

[SPARK-14451][SQL] Move encoder definition into Aggregator interface #12231

rxin wants to merge 8 commits into from

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Apr 7, 2016

What changes were proposed in this pull request?

When we first introduced Aggregators, we required the user of Aggregators to (implicitly) specify the encoders. It would actually make more sense to have the encoders be specified by the implementation of Aggregators, since each implementation should have the most state about how to encode its own data type.

Note that this simplifies the Java API because Java users no longer need to explicitly specify encoders for aggregators.

How was this patch tested?

Updated unit tests.

@rxin
Copy link
Contributor Author

rxin commented Apr 7, 2016

cc @marmbrus @cloud-fan

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55186 has finished for PR 12231 at commit c0379a2.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class Aggregator[-IN, BUF, OUT] extends Serializable

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 7, 2016

Is it possible that users want to use one Aggregator in different context with different encoders?

@rxin rxin mentioned this pull request Apr 7, 2016
Closed
@rxin
Copy link
Contributor Author

rxin commented Apr 7, 2016

I don't know -- it's possible but I don't know how often you'd see that ...

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55189 has finished for PR 12231 at commit 54d75a3.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55194 has finished for PR 12231 at commit 3761737.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

asfgit pushed a commit that referenced this pull request Apr 7, 2016
@rxin
[SPARK-14452][SQL] Explicit APIs in Scala for specifying encoders
e11aa9e 
## What changes were proposed in this pull request?
The Scala Dataset public API currently only allows users to specify encoders through SQLContext.implicits. This is OK but sometimes people want to explicitly get encoders without a SQLContext (e.g. Aggregator implementations). This patch adds public APIs to Encoders class for getting Scala encoders.

## How was this patch tested?
None - I will update test cases once #12231 is merged.

Author: Reynold Xin <[email protected]>

Closes #12232 from rxin/SPARK-14452.
@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55191 has finished for PR 12231 at commit 609e8eb.

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #2766 has finished for PR 12231 at commit 3761737.

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2016

Test build #55202 has finished for PR 12231 at commit 7a09996.

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

@SparkQA
Copy link

SparkQA commented Apr 8, 2016

Test build #55299 has finished for PR 12231 at commit ca1b47e.

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

@SparkQA
Copy link

SparkQA commented Apr 8, 2016

Test build #55315 has finished for PR 12231 at commit b58675a.

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

@SparkQA
Copy link

SparkQA commented Apr 8, 2016

Test build #55316 has finished for PR 12231 at commit b759f0a.

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

cloud-fan
cloud-fan reviewed Apr 8, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/typedaggregators.scala
override def merge(b1: Double, b2: Double): Double = b1 + b2
override def finish(reduction: Double): Double = reduction

override def bufferEncoder: Encoder[Double] = ExpressionEncoder[Double]()
Copy link
Contributor

@cloud-fan cloud-fan Apr 8, 2016

Choose a reason for hiding this comment

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

use Encoders.scalaDouble?

Copy link
Contributor Author

@rxin rxin Apr 9, 2016

Choose a reason for hiding this comment

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

this is internal, so it is not that bad to use the internal api.

@cloud-fan
Copy link
Contributor

cloud-fan commented Apr 8, 2016

LGTM

@rxin
Copy link
Contributor Author

rxin commented Apr 9, 2016

Merging in master. Thanks!

@asfgit asfgit closed this in 520dde4 Apr 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rxin @SparkQA @cloud-fan