-
-
Notifications
You must be signed in to change notification settings - Fork 504
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
Implement column-as keyword for 'select' with SelectAs(). #499
base: master
Are you sure you want to change the base?
Implement column-as keyword for 'select' with SelectAs(). #499
Conversation
When I see So all the I think keeping the name as is is more appropriate. |
0dd0338
to
5991a60
Compare
This is a good point, I've rebased on This rename makes more sense in the context of a future PR, which adds support for selecting aggregates as columns, so you can for example do: select "Supplier", count("Customer") as "Num Customers"
from "Cases"
group by "Supplier"
order by "Num Customers" desc We chose the For that PR, what would be a good way (name) to make the distinction between what --edit |
@dgeelen-uipath I still see that the aggregate methods names are flipped like |
5991a60
to
3d14ba8
Compare
No, you're right. This PR was intended as preparatory work for the follow up (#504), but it's diverging a bit it would seem (that's okay, but something to keep in mind). There were multiple renames in this PR; one to rename I've changed this PR as follows; I've kept the original functions ( However, I'm personally not entirely satisfied with the result, I'm wondering if the alias support could be folded in to the existing functions as a default argument instead. The problem with that is that Perhaps we can keep the naming for the What do you think? P.S. I will update #504 once it's clear how best to deal with the naming, so for now it will remain a bit out-of-date. |
This adds support for the following two syntaxes:
SelectAs()
Aggregates aliases
SelectAggregate()
supersedesAsAggregate()
, e.g.AsCount()
becomesSelectCount()
.Notes
While new function is designed as a drop-in replacement for the old one, we still opted to rename the existing aggregates from e.g.
AsCount()
toSelectCount()
, as we felt that this name more clearly communicates the intended use; to 'select' an 'aggregate' column. We considered keeping the old names around for compatibility, but then there would be two confusingly named functions with different capabilities and behaviour.We noticed that there is existing support for the
as
keyword, where a lower-caseas
in a column name is treated as theAS
keyword, which this PR leaves intact. However note that the existing way of handling 'as' is fragile. For example this does not work properly for aggregates, as in the following example.This yields invalid SQL syntax where there is an additional
as
added within thecount()
function.