Skip to content

Conversation

@davies
Copy link
Contributor

@davies davies commented Nov 18, 2015

BytesToBytesMap will be used by UnsafeHashRelation, and stored in memory store, then SizeEstimator will see BlockManager as part of BytesToBytesMap, which is not what we expect.

This PR remove the reference to BlockManager in BytesToBytesMap, using SparkEnv.get().blockManager during spilling (currently we get the blockManager in one of the constructors).

cc @yhuai @rxin

@SparkQA
Copy link

SparkQA commented Nov 18, 2015

Test build #46191 has finished for PR 9799 at commit 907ec97.

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

@JoshRosen
Copy link
Contributor

Just curious: might it be less brittle to implement the new size estimation interface instead, since we should be able to figure out the size exactly?

@andrewor14
Copy link
Contributor

+1 to @JoshRosen's suggestion, which is actually @davies' original proposal. Even though we don't use broadcast BytesToBytesMap currently I think it's still a good guard against potential issues. This patch in its current state actually removes some test logic, which can be avoided if we provide a precise estimate ourselves instead.

@davies
Copy link
Contributor Author

davies commented Nov 18, 2015

Since this is not the approach everybody like, and SPARK-11792 is already resolved, I'd close this PR for now.

@davies davies closed this Nov 18, 2015
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