[SPARK-12688][SQL] Fix spill size metric in unsafe external sorter

[carsonwang]

When doing a sql aggregation in tungsten mode, the spill size metric may not update though there are data spilled. Because the data are stored in the BytesToBytesMap instead of UnsafeExternalSorter, the memory are freed when the map resets instead of when the sorter spills.

[SparkQA]

Test build #48913 has finished for PR 10634 at commit 2fd3ab6.

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

[SparkQA]

Test build #48915 has finished for PR 10634 at commit 81e3227.

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

[SparkQA]

Test build #48923 has finished for PR 10634 at commit be67488.

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

[SparkQA]

Test build #48992 has finished for PR 10634 at commit 416d73d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class JavaSampleActorReceiver<T> extends JavaActorReceiver
  • public class JavaActorWordCount
  • abstract class ActorReceiver extends Actor
  • abstract class JavaActorReceiver extends UntypedActor

[JoshRosen]

According to my understanding of spill metrics, a spill needs to update both memoryBytesSpilled and diskBytesSpilled. The memory spill is the in-memory size of the data being spilled, while the disk spill records the size of that data after it has been serialized and written to disk. As a result, I think that there must be a corresponding incDiskBytesSpilled call somewhere. I'm thinking that this memory spill metric should be updated closer to the site of where we increment the disk bytes spilled rather than here, since I think doing it that way would make it easier to reason about whether we're double-counting.

If this does turn out to be the right place for this spill, it would be great to add a code comment explaining the rationale for why this call must be here.

[JoshRosen]

Ping @carsonwang, do you plan to update this PR to address my comment?

[carsonwang]

Sorry for the delay, @JoshRosen . I will update this soon.

[carsonwang]

retest this please

[SparkQA]

Test build #49974 has finished for PR 10634 at commit 4486071.

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

[carsonwang]

@JoshRosen , I now also update diskBytesSpilled. Previously it is not updated for aggregation. Please help review this.

[carsonwang]

For Sql aggregation, the spillSize here is 0 because the data are stored in a map instead of this sorter. So incMemoryBytesSpilled(spillSize) actually increase 0. We need update the MemoryBytesSpilled after freeing the memory in the map.

[carsonwang]

retest this please

[SparkQA]

Test build #49976 has finished for PR 10634 at commit 4486071.

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

[SparkQA]

Test build #49984 has finished for PR 10634 at commit d689873.

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

[SparkQA]

Test build #49983 has finished for PR 10634 at commit 4486071.

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

[carsonwang]

@JoshRosen , do you have any further comments?

[carsonwang]

/cc @cloud-fan @andrewor14 , did you guys see spill size > 0 when the UI was introduced? Can you take a look at this fix?

[cloud-fan]

is it possible to write a test for this bug?

[SparkQA]

Test build #50857 has finished for PR 10634 at commit 4cc0862.

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

[rxin]

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.