[BEAM-9547] Add basic support for DataFrame.{eval,query}#13264
[BEAM-9547] Add basic support for DataFrame.{eval,query}#13264TheNeuralBit merged 6 commits intoapache:masterfrom
DataFrame.{eval,query}#13264Conversation
There was a problem hiding this comment.
Currently if the user specifies a local variable in expr with @<local> this will fail at construction time (when generating the proxy), but the error message isn't helpful, it just indicates <local> doesn't exist.
There was a problem hiding this comment.
(I'll update the XXX with a jira number before merging if this gets approved, I just didn't want to create it prematurely)
There was a problem hiding this comment.
Go ahead and create the jira. Can we give a better message if @ is in the expression?
There was a problem hiding this comment.
Yeah I'll add a regex to check for @<py identifier> and point to the jira.
TheNeuralBit
force-pushed
the
eval-query
branch
from
8eed466 to
cb6a5bb
Compare
|
R: @robertwb |
| @functools.wraps(func) | ||
| def wrapper(self, inplace=False, **kwargs): | ||
| result = func(self, **kwargs) | ||
| def wrapper(self, *args, inplace=False, **kwargs): |
There was a problem hiding this comment.
This is dangerous, as it won't catch if inplace was passed as a non-keyword argument.
There was a problem hiding this comment.
That could happen only with methods where the first kwarg is inplace right? I wouldn't think there's many of those but I suppose there could be.
So what's the right thing to do here? We could use inspect to find inplace in the args/kwargs of func and handle it correctly. Or more simply just never use this in methods where we need to pass through args.
There was a problem hiding this comment.
We shouldn't use this in places where we may need to pass args. (Actually, if we're just passing a*args, we can't generally detect inplace safely at all).
There was a problem hiding this comment.
If we passed the base type as in populate_defaults we could inspect it's method. Seems like a lot of trouble.. but I'm just worried that there's some edge case where we're dropping non-keyword args, and it would be great to catch that possibility at import time.
There was a problem hiding this comment.
I removed this change and instead handle inplace in the new methods, PTAL
There was a problem hiding this comment.
Go ahead and create the jira. Can we give a better message if @ is in the expression?
This reverts commit cb6a5bb.
2 participants
Adds support for executing
evalandqueryas element-wise methods. Restricts most kwargs to their defaults for simplicity, only parser, engine and truediv are still allowed to be changed.This PR also includes a commit to pass
*argsthroughmaybe_inplaceas not having it breakseval.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.