-
Notifications
You must be signed in to change notification settings - Fork 358
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.
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
DataFrame.cache() #397
DataFrame.cache() #397
Conversation
Codecov Report
@@ Coverage Diff @@
## master #397 +/- ##
==========================================
+ Coverage 93.12% 93.15% +0.02%
==========================================
Files 28 28
Lines 3448 3462 +14
==========================================
+ Hits 3211 3225 +14
Misses 237 237
Continue to review full report at Codecov.
|
@HyukjinKwon, I have changed the implementation from using The reason for the change of implementation is that
But in the current implementation, I am returning the |
@HyukjinKwon, @ueshin can you please look into my PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to expose __enter__
and __exit__
in DataFrame
directly since we might want to use them in the different case in the future.
How about introducing _CachedDataFrame
extending DataFrame
and supply __enter__
, __exit__
, and unpersist
?
class DataFrame(...):
...
def cache(self):
return _CachedDataFrame(self._sdf, ...)
...
class _CachedDataFrame(DataFrame):
def __init__(self, sdf, ...):
self._cached = sdf.cache()
super(_CachedDataFrame, self).__init__(self._cached, ...)
def __enter__(self):
return self
def __exit__(self, ...):
self.unpersist()
def unpersist(self):
if self._cached.is_cached:
self._cached.unpersist()
I might miss something, though.
WDYT? cc @HyukjinKwon
Hi @ueshin, Please review my changes. I am happy to help regarding any further issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left some comments and otherwise LGTM.
I'd like to hear opinions from @HyukjinKwon, so I'd leave it to him.
Hi @ueshin, |
Softagram Impact Report for pull/397 (head commit: d9ef2c7)⭐ Change Overview
📄 Full report
Give feedback on this report to [email protected] |
cc: @HyukjinKwon, can you please look into this? |
Sorry for late response. yup, I am taking a look now :). |
This PR proposes DataFrame.cache() API.
Resolves #333