Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Aug 6, 2015

This PR adds SQLMetric/SQLMetricParam/SQLMetricValue to specialize accumulators to avoid boxing. All SQL metrics should use these classes rather than Accumulator.

@zsxwing
Copy link
Member Author

zsxwing commented Aug 6, 2015

cc @rxin

@SparkQA
Copy link

SparkQA commented Aug 6, 2015

Test build #40038 has finished for PR 7996 at commit 42f50c3.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Found"

Copy link
Contributor

Choose a reason for hiding this comment

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

if i understand this correctly, this only works because the zero value of all these metrics are naturally 0 right?

(since we set zero value while we deserialize data)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I think we don't need a different initial value for SQL metrics. So just avoid to add an initial value to SQLMetricParam.

@rxin
Copy link
Contributor

rxin commented Aug 7, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Aug 7, 2015

Test build #40113 has finished for PR 7996 at commit 14a5f0a.

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

@zsxwing
Copy link
Member Author

zsxwing commented Aug 7, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Aug 7, 2015

Test build #40126 has finished for PR 7996 at commit 14a5f0a.

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

@SparkQA
Copy link

SparkQA commented Aug 7, 2015

Test build #1400 has finished for PR 7996 at commit 14a5f0a.

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

@rxin
Copy link
Contributor

rxin commented Aug 7, 2015

Thanks - merging this in.

asfgit pushed a commit that referenced this pull request Aug 7, 2015
…xing

This PR adds SQLMetric/SQLMetricParam/SQLMetricValue to specialize accumulators to avoid boxing. All SQL metrics should use these classes rather than `Accumulator`.

Author: zsxwing <[email protected]>

Closes #7996 from zsxwing/sql-accu and squashes the following commits:

14a5f0a [zsxwing] Address comments
367ca23 [zsxwing] Use localValue directly to avoid changing Accumulable
42f50c3 [zsxwing] Add SQLMetric to specialize accumulators to avoid boxing

(cherry picked from commit ebfd91c)
Signed-off-by: Reynold Xin <[email protected]>
@asfgit asfgit closed this in ebfd91c Aug 7, 2015
@zsxwing zsxwing deleted the sql-accu branch August 7, 2015 07:44
CodingCat pushed a commit to CodingCat/spark that referenced this pull request Aug 17, 2015
…xing

This PR adds SQLMetric/SQLMetricParam/SQLMetricValue to specialize accumulators to avoid boxing. All SQL metrics should use these classes rather than `Accumulator`.

Author: zsxwing <[email protected]>

Closes apache#7996 from zsxwing/sql-accu and squashes the following commits:

14a5f0a [zsxwing] Address comments
367ca23 [zsxwing] Use localValue directly to avoid changing Accumulable
42f50c3 [zsxwing] Add SQLMetric to specialize accumulators to avoid boxing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants