Allow for alias omission in aggregate expressions

[jpcody]

This fix should be backwards-compatible, but I'm not confident it's being applied at the appropriate level.

When using aggregate expressions in a HAVING claus, an alias was forced, causing a syntax error in Postgres. The following Arel:

posts    = Arel::Table.new(:posts)
comments = Arel::Table.new(:comments)

posts.
  join(comments).on(posts[:id].eq(comments[:post_id])).
  having(comments[:rating].average.gt(90)).
  project(Arel.star).
  to_sql

Produces:

SELECT * FROM "posts" INNER JOIN "comments" ON "posts"."id" = "comments"."post_id" HAVING AVG("comments"."rating") AS avg_id > 90

Which leads to the following error: ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR: syntax error at or near "AS" because Postgres (and maybe others?) disallows aliases in HAVING clauses.

This fix allows these aggregate functions to take an optional argument that can either override the avg_id default string or be nil to skip the alias altogether, while maintaining the default behavior.

Now we could do:

posts    = Arel::Table.new(:posts)
comments = Arel::Table.new(:comments)

posts.
  join(comments).on(posts[:id].eq(comments[:post_id])).
  having(comments[:rating].average(nil).gt(90)).
  project(Arel.star).
  to_sql

To yield:

SELECT * FROM "posts" INNER JOIN "comments" ON "posts"."id" = "comments"."post_id" HAVING AVG("comments"."rating") > 90

And avoid syntax errors.

[rafaelfranca]

While I agree with this patch I find that passing nil as argument does say what that argument does. For me it seems it is trying to calculate the average of nil. Maybe we should take a hash with a alias option.

cc @tenderlove

[matthewd]

IMO this is rather back-to-front... the alias shouldn't be part of the aggregate to begin with: it's properly part of the 'project' production, not the function call. And contrariwise, our motivation for applying the alias there (predictability of the output column name, I guess?) should presumably apply equally to any non-aggregate complex expression.

[jpcody]

@matthewd Agreed, the alias inclusion felt odd to begin with, particularly because Arel makes affordances for explicit aliases. But it seemed to have been there since nearly the beginning of time, so I was gun-shy on touching it.

I could remove the alias and ensure the tests still pass, and maybe @tenderlove as the original author of that would be able to hit the brakes if that seems to violate the original motivation?

[jpcody]

OK, I had some more time this morning, and I've removed the default aggregate aliases altogether. There were some FIXMEs asking if this was still needed, and it's my opinion they're not. Because aliasing with as on aggregates is totally possible, it doesn't make sense to default to behavior that breaks having clauses.

The only other thing I can think of is that there are occasions I'm unaware of where generating an alias is necessary on aggregate functions, but Googling around turns nothing up there.

(It's worth noting that this change could very-well break existing code that relies on the aliases that are generated.)

[jpcody]

@rafaelfranca I know you're quite busy with 4.2 betas (thanks and kudos!), but I'm curious if there's any more I can do to help this one across the finish line?

[rafaelfranca]

I'll defer to @matthewd

[matthewd]

I think it's fair to treat the name as undefined here, given that DBs have differing opinions on the default; if you need a particular value, you should specify an explicit alias. (Or just access the column by position, as we do in Rails.)