-
Notifications
You must be signed in to change notification settings - Fork 550
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
Do not cast decimal to integer #563
base: master
Are you sure you want to change the base?
Conversation
Looks like CI failure is not related? |
Yes, the CI issue was unrelated. I just fixed it in e91d8ed. Since this behavior has been in 0.3.x for a long time, I'm going to tag the revert for 0.4.0. I'm concerned if there will be other fallout / what motivated the change in the first place. |
@sodabrew as per author's commit message,
Obviously this was the initial intention – to not end up having decimals returned, once aggregating SUM in MySQL. However, to me sticking with whatever types MySQL returns, would mean "playing by MySQL's rules". Conforming with MySQL sounds like the rights thing to do, as it reduces possible side effects like the one we've seen here. |
I read that in the commit but it didn't compute it my head :) I found it in the docs: http://dev.mysql.com/doc/refman/5.1/en/group-by-functions.html
Well, on the one hand I agree that we should play by MySQL's rules. On the other hand, it seems more surprising to get a DECIMAL back from SUM(bunch of integers) and probably a more common trip-up for users. I wonder if there's a reasonable workaround to get both behaviors. |
I'm not really sure how to proceed here. I don't have a good sense if people are now expecting the >=0.3.11 behavior, or if it's reasonable to choose the behavior as a query_option. I suppose the simplest thing would just me to merge this and see if anybody shouts about 0.4.0... 🙊 Could I ask you to rebase this against current master, and to add a unit test? |
This reverts a099e55, where regression has been introduced.
Fixes #557.