Skip to content

[SPARK-31253][SQL][FOLLOW-UP] simplify the code of calculating size metrics of AQE shuffle #28240

Closed
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:refactor
Closed

[SPARK-31253][SQL][FOLLOW-UP] simplify the code of calculating size metrics of AQE shuffle #28240
cloud-fan wants to merge 1 commit intoapache:masterfrom
cloud-fan:refactor

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

A followup of #28175:

  1. use mutable collection to store the driver metrics
  2. don't send size metrics if there is no map stats, as UI will display size as 0 if there is no data
  3. calculate partition data size separately, to make the code easier to read.

Why are the changes needed?

code simplification

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing tests

@cloud-fan
Copy link
Contributor Author

cc @JkSelf @maryannxue

val partitionDataSizeMetrics = metrics("partitionDataSize")
driverAccumUpdates ++= dataSizes.map(partitionDataSizeMetrics.id -> _)
// Set sum value to "partitionDataSize" metric.
partitionDataSizeMetrics.set(dataSizes.sum)
Copy link

Choose a reason for hiding this comment

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

why use the dataSizes's sum not partitionDataSizes's sum ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

partitionDataSizes.foreach { dataSizes => so dateSizes = partitionDataSizes.get

@SparkQA
Copy link

SparkQA commented Apr 17, 2020

Test build #121405 has finished for PR 28240 at commit 3092cd8.

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

Copy link
Contributor

@maryannxue maryannxue left a comment

Choose a reason for hiding this comment

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

LGTM.

@gatorsmile
Copy link
Member

Thanks! Merged to master/3.0

@jiangxb1987
Copy link
Contributor

Merged to master only, please manually backport this to 3.0. Thanks!

@cloud-fan
Copy link
Contributor Author

We haven't decided to backport the AQE SQL metrics yet, so this PR can't be backported for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments