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

Add ids as a function alias #4

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

Conversation

StefSchenkelaars
Copy link
Contributor

Hi there, I found myself fetching the associated ids during a call during my project multiple times. I performed this by calling

Order.group(:currency).calculate_all(ids: 'ARRAY_AGG(id)')

I've now added the :ids as a function alias but the ARRAY_AGG function only works on Postgres.

On MySQL sort of similar behaviour can be achieved by using the group-concat function but it returns a string instead of an array. This can later be parsed but I did not put too much effort in this since I don't use MySQL. In addition you would need a check in the helpers module to see what database service you are using. Maybe someone has an idea and we can have a discussion about this.

@codesnik
Copy link
Owner

Um, do we really need it?

I mean, compare with other "function aliases": ids isn't working for any column other than id, it works only on postgres, and it isn't actually a "calculating" aggregate, calculate_all name kinda stop making sense.

I've added function aliases (and calculate_all gem itself) for cases when you going to fetch a lot of aggregates in one pass, like calculate_all(:amount_max, :amount_min, :amount_average, :amount_sum). I can't imagine a scenario, when you would want to fetch more aggregates together with calculate_all('array_agg(id)') or calculate_all(ids: 'array_agg(id)'), and it's short enough already with one argument, should work as a replacement for grouping #ids on scopes.

I actually considered it a year ago, but didn't add it because of reasons above. There're a lot of nice functions in https://www.postgresql.org/docs/9.6/static/functions-aggregate.html (I love percentiles, for example), but they are all made possible to be used by this gem already.

I would consider adding postgres only array_agg alias, though, if it'd be more generic.
Something enabling using different columns, calling array_agg(disctint column1), or even array_agg(column1 order by column2 desc), though. Maybe, :column1_array or :column1_distinct_array, or :column1s. But it's a bit too magic already. What do you think?

@codesnik
Copy link
Owner

Also, I never considered #calculate_all a good method to be called from your views or controllers. It's a plumbing, to be called from model class methods or some query object. And if you extracted some logic there, 'array_agg()' sql fragment doesn't look so scary anymore.

@StefSchenkelaars
Copy link
Contributor Author

Well I of course don't use it with only fetching the ids. I use it in an API call which returns multiple averages and ids grouped by some attribute. So I do use it now in a combination with other aggregates.

But I agree that it is not really generic and if you start on adding this alias, you should probably include more postgres aggregate functions. Maybe a good idea to leave this PR open for now and not use it until I / we have found a nice generic way of combining this stuff.

On the other hand, things like :column1_array or :column1s doesn't sound that magic for me and now you just get an error if the function alias is not found so I think more aliases is not bad. You can always use custom stuff by entering a string.

By the way, did you also wonder why this is not included in ActiveRecord? Especially that the calculate functions like average do not accept arrays and only work on one attribute.

@codesnik
Copy link
Owner

I've thought about making a patch for active_record, as an additional form to call #calculate or as another method. This gem was my try to refine and strip down functionality before trying to incorporate it into activerecord. That's one of the reasons I've been reluctant to add postgres only features.

But I feel it doesn't really fit, at least, right now.

ActiveRecord isn't really made with using raw sql fragments in mind, and aggregating stuff is so basic, even with this gem it isn't advanced much into the relation realm. Looks like anyone who needs something more complex than CRUD has to juggle with arrays of hashes of arrays returned by a private activerecord API, or jump straight to sequel or alternatives.

That gem in particular was extracted from code in which I tried to use more advanced stuff, see http://stackoverflow.com/questions/35210823/why-could-postgresql-9-5s-cube-rollup-and-grouping-sets-be-slower-than-equival/35544953

I just noticed that first particular step could be implemented with public API #pluck, and wanted to share this hack with others, it's nowhere near obvious.
And still after initial grouping I had to juggle with arrays and hashes, like it isn't an ActiveRecord at all.

It's not included or written by someone else because not a lot of people need it, I think. Do you know any alternatives for this gem? And you are probably the only active user of this one.

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.

2 participants