querybuilder icon indicating copy to clipboard operation
querybuilder copied to clipboard

Implement column-as keyword for 'select' with SelectAs().

Open dgeelen-uipath opened this issue 3 years ago • 4 comments

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.

dgeelen-uipath avatar Jun 22 '21 07:06 dgeelen-uipath

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.

ahmad-moussawi avatar Jun 23 '21 15:06 ahmad-moussawi

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

dgeelen-uipath avatar Jun 25 '21 14:06 dgeelen-uipath

@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?

ahmad-moussawi avatar Jul 01 '21 08:07 ahmad-moussawi

@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.

dgeelen-uipath avatar Jul 02 '21 13:07 dgeelen-uipath