Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dgeelen-uipath
Copy link
Contributor

This adds support for the following two syntaxes:

SelectAs()
Query().SelectAs(("Row1", "Alias1"), ("Row2", "Alias2")).From("Table");
--> "SELECT [Row1] AS [Alias1], [Row2] AS [Alias2] FROM [Table]"
Aggregates aliases

SelectAggregate() supersedes AsAggregate(), e.g. AsCount() becomes SelectCount().

Query().SelectCount("Column", "Alias").From("Table");
--> "SELECT COUNT([Column]) AS [Alias] FROM [Table]"

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() to SelectCount(), 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-case as in a column name is treated as the AS 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.

Query().AsCount(new[] { "Column as Alias" }).From("Table");
--> "SELECT COUNT([Column] AS [Alias]) AS [count] FROM [Table]"

This yields invalid SQL syntax where there is an additional as added within the count() function.

@ahmad-moussawi
Copy link
Contributor

When I see SelectCount or similar functions, I get the intention that this is a shortcut for SelectRaw("count(...)") method while this is not the case, the AsCount here is the non executable version of XQuery's Count() function which execute the query and return the result immediately.

So all the AsX methods here are just to inform the compiler that this query will be used as Count() when executed so non necessary information can be trimmed out from the generated SQL.

I think keeping the name as is is more appropriate.

@dgeelen-uipath dgeelen-uipath force-pushed the implement_as_keyword_support_for_aggregates branch from 0dd0338 to 5991a60 Compare June 25, 2021 14:24
@dgeelen-uipath
Copy link
Contributor Author

dgeelen-uipath commented Jun 25, 2021

When I see SelectCount or similar functions, I get the intention that this is a shortcut for SelectRaw("count(...)") method while this is not the case, the AsCount here is the non executable version of XQuery's Count() function which execute the query and return the result immediately.

So all the AsX methods here are just to inform the compiler that this query will be used as Count() when executed so non necessary information can be trimmed out from the generated SQL.

I think keeping the name as is is more appropriate.

This is a good point, I've rebased on master and dropped the rename for now.

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 SelectCount() name more for this case, with the AsCount() behaviour as a special case for only the case where the aggregation is COUNT and more than two columns are selected.

For that PR, what would be a good way (name) to make the distinction between what AsCount() does vs. selecting aggregates of columns?

--edit
For context I've opened #504 which implements selecting multiple aggregate columns and should fix e.g. #203

@ahmad-moussawi
Copy link
Contributor

@dgeelen-uipath I still see that the aggregate methods names are flipped like CountAs instead of AsCount or maybe I am browsing the wrong branch?

@dgeelen-uipath dgeelen-uipath force-pushed the implement_as_keyword_support_for_aggregates branch from 5991a60 to 3d14ba8 Compare July 2, 2021 12:27
@dgeelen-uipath
Copy link
Contributor Author

@dgeelen-uipath I still see that the aggregate methods names are flipped like CountAs instead of AsCount or maybe I am browsing the wrong branch?

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 AsCount() -> CountAs() and then another to rename CountAs() -> SelectCount(). Only the latter rename was moved to #504 and I forgot to undo the CountAs() rename here. Sorry for the confusion.

I've changed this PR as follows; I've kept the original functions (AsCount(), AsSum(), ...), but added new functions which support a column alias (AsCountAs(), AsSumAs(), ...). This should hopefully make it more clear what is going on with the code changes.

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 AsCount() already takes a list of strings, which it uses to implement the counting of multiple columns, and thus there is no place to add the alias. I do believe that this is a problem only for COUNT, since the other aggregates don't have the special case support for multiple columns, so perhaps we could make it an exception for AsCountAs() only? It doesn't seem very uniform though, and maybe even doesn't help with the difference in behaviour between AsAggregate() functions and the proposed SelectAggregate() from #504 (which implements support for multiple aggregates per query).

Perhaps we can keep the naming for the AsCount()/AsCountAs() like here, and then introduce a name SelectCount() (with alias support included) for the different behaviour implemented by #504. Another possibility could be to drop the alias support for the AsAggregate() functions all together, and focus on supporting aliases for multiple-aggregates-per-query only.

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.

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.

None yet

2 participants