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
[ENH] Column aliasing #3333
[ENH] Column aliasing #3333
Conversation
@samukweku So what if we only introduce an f-method Otherwise, my feeling is that
is not much better than just
The latter even looks cleaner to me. Btw, one of the tests is failing due to some documentation variable being missing. |
@oleksiyskononenko that is so much better/cleaner and was the original idea of the PR. i just could not figure out how to write methods off the f symbol |
@oleksiyskononenko how do I go about implementing |
@samukweku I guess you already implemented many of f-methods here: https://github.com/h2oai/datatable/blob/main/src/core/expr/fexpr.cc However, your implementation was normally
Here you won't be able to do this, because we're not going to implement any new |
So essentially you just need to move your implementation from static py::oobj pyfn_alias(const py::XArgs &args) {
YOUR_IMPLEMENTATION
} to oobj PyFExpr::alias(const XArgs& args) {
YOUR_IMPLEMENTATION
} and make some other adjustments, so that it works. |
been busy these past days on some other stuff ... I'll try and resume on this on the weekend |
@oleksiyskononenko I'm not even sure what I'm doing here; I don't know how to directly set up a method on the |
@samukweku Sure, let me rewrite what you've got here, so that you will see. |
I will greatly appreciate that @oleksiyskononenko |
@samukweku So I've fixed what you currently have to only support the
|
@oleksiyskononenko having it as a method is better and cleaner; it will be in the same way as remove/ extend. |
Having it as an f-method and also as dt function will not probably heart. I'm also starting to think that since we already have both |
Not sure |
@oleksiyskononenko if we leave as is (allowing for both a function and a method), are we agreed on what the name should be? |
My feeling is that
Since we can't use |
as, as used in SQL, is to rename a column, so we are not far off if we use rename/alias Also with this function we can rename columns and alias them. So it does not matter much IMO. alias just seems more direct - basically saying |
@samukweku yeah, let me push updates to this PR to fix couple of things, then we could decide on the name. |
So I've re-factored the function, while keeping the name Please see if everything makes sense to you and don't hesitate to ask any questions. What we're missing here is documentation and also the final concord on the function name. While we can leave it as One of the options for the name I came with is |
Thanks @oleksiyskononenko . I believe your name suggestion ( |
@oleksiyskononenko I think we should go ahead and use the |
@samukweku That's not a big deal to have only the f-method, however, I don't think it will hurt if we keep a dt function also. For instance, |
The only advantage I see of keeping this as an f-method is that we could then support
to
Then, instead of doing |
@oleksiyskononenko the variable args is great! Totally forgot about it. I think it is a great idea for both function and method. I think |
Unfortunately, with the function |
Does |
I'll add the docs and make changes, while changing to |
@samukweku I guess you can leave it as |
@samukweku I found that I have already created documentation, just had to commit the files. Please take a look and if it's ok, we can merge this PR. |
My bad @oleksiyskononenko I was having a look at it. Glad it has been merged though - it is a superb addition to the library |
@samukweku oops, sorry, I saw your thumbs up and thought you already finished with the review. Anyways, please continue your review and if you see anything wrong or needs clarification, please open a separate PR with additions. |
This PR implements column's aliasing as proposed in #2684. We couldn't name the method
.as()
though, becauseas
is a built-in python keyword — hence, we use.alias()
instead. Column aliasing is now also available in the group-by clause.Closes #2504