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

consistent naming in aggregate functions #63 #65

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

Conversation

ryskajakub
Copy link
Contributor

Also, at least in 9.3.6, the StdP etc. is not referring to any aggregate function. I don't know if was a alias, which is removed now or it's just broken or else.

@tomjaguarpaw
Copy link
Owner

@tomjaguarpaw
Copy link
Owner

I fear this will require another typeclass.

@ryskajakub
Copy link
Contributor Author

I added constraints for all the currently publicly implemented aggregations, hopefully that should fill any type hole that is currently there.

I'm not sure if the naming is best, it also could be in another module?

@tomjaguarpaw
Copy link
Owner

Need to get rid of aggregateCoerceFIXME in Test.hs.

@ryskajakub
Copy link
Contributor Author

Yes. Removed.

@tomjaguarpaw
Copy link
Owner

This looks like the right idea. Let me give it some more thought.

@tomjaguarpaw
Copy link
Owner

Coming back to this, it is good work but I am concerned about the proliferation of type classes. I will leave it as an open PR for further discussion.

@hesselink
Copy link
Contributor

I just ran into this, trying to average an integer column. What is your thinking on this now?

@tomjaguarpaw
Copy link
Owner

Hi @hesselink, can you clarify what you mean? You used avg on an int column and hit a runtime error because the result was numeric not int?

@hesselink
Copy link
Contributor

I'm running avg on an int column, and it doesn't type check since opaleye says it can only be applied to PGFloat8. I've just hit #117 as well, doing a sum on an int8 column and getting a numeric, which gave a runtime error since opaleye thought I'd be getting back another int8.

@tomjaguarpaw
Copy link
Owner

I see. The workaround (which you probably already know) is to use unsafeCoerceColumn.

I don't know whether the best solution is to use a typeclass like @coubeatczech's implementation or to provide several differently-named functions. I don't really like proliferating the number of typeclasses but perhaps it's neater overall.

What do you think?

@hesselink
Copy link
Contributor

I think a type class with a functional dependency like @coubeatczech created, or the equivalent type family, is the neatest solution and maps very cleanly to the way postgres treats the output types of functions: they depend on the input types. But I know you are worried about complicating the types. I'm not sure how differently named functions would work here, though. You'd need a different aggregator for each input type, right?

@tomjaguarpaw
Copy link
Owner

Yes, I was thinking of something clunky like avgInt4, avgInt8, avgFloat4 each specialised to the relevant input type.

@hesselink
Copy link
Contributor

That's indeed a bit clunky. Like I said I prefer the classes (also over type families I think, since it matches nicely with contraints like StringAgg that are not really type functions), but I also wouldn't really mind having a gazillion aggregation functions. Both of those are much better than the current situation.

One advantage of class + FD is that it can also be used for operators, whereas creating different versions there will get really ugly: you'll either get ascii soup, or you have to stop using operators for the less common types.

@hesselink
Copy link
Contributor

Another advantage: it's mostly backwards compatible, in that people who are using the operations at correct types already aren't going to have to change their code. With the different specializations, they'll have to look at the code to see at what types they're using them, and change the names accordingly.

@tomjaguarpaw
Copy link
Owner

OK, since @coubeatczech already implemented this and chose typeclasses we can go for that. It needs rebasing though, which either I will do when I get round to it or you can do and send me a new PR.

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.

3 participants