-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Replace decimal(12,4) with decimal(9,3) and decimal(13,3) #636
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
Comments
@colinmollenhour thanks. I put it into our development queue |
@colinmollenhour @ilol |
So... (13,3) or (12,4)? IRR exchange rate 1 IRR = 0,0000283 EUR |
So it looks like we have uncovered a separate issue.. Currently the I am not experienced with multi-currency so perhaps the exchange rate being >3 decimals is a valid reason for using >3 throughout, but are not the final amounts rounded anyway? E.g. if my total is 1 EUR and I purchase in RON, will I actually be invoiced for 4.4244 RON or 4.424 RON? I am questioning if all of the totals really need >3. AFAIK my credit card company has never billed me for a fraction of a cent before, but maybe that differs in other countries? Regardless, if a case can be made for 4 decimals then 13,4 would make the most sense (13 consumes the same storage as 12). 13,4 means 13 total digits, 4 of which are decimal places. EDIT: I understand rounding issues during calculations and I am not talking about rounding to 3 decimal places for mathematical operations, just for storage. |
@colinmollenhour I feel that my explanation was not complete. So here are the details. And here is an idea to avoid rounding issues. |
@tzyganu, intelligible explanation. It make sense. |
Bah, I missed the UVR currency.. I do think that a dynamic solution would be nice. E.g. allow the user to select for the following:
I personally would then choose 9,2 - 9,5 and 13,2 but someone else may choose 13,4 - 13,7 and 16,4 for example. |
I just found a flaw in my "unfinished idea". Even if the dynamic solution for decimal columns is not considered, I still think that a separate table for price attribute values only is needed. Could make the indexing a bit faster. Should I open an other issue for this? or is it OK if I leave it here? |
When the decimals are changed from 4 to 3 oder 2 digits I think the rounding will be affected, too. Magento had decent rounding problems in the past and right now it looks like they're solved. Changing the decimal storage might bring back all the pain we had in the last years with PayPal, taxes, the missing cent and order export to backend systems. |
@tzyganu That is a good point. However, in real world I don't think there is much use for decimal types other than monetary amounts. Text fields can store as many decimals as one needs. In one store I manage with 140 custom attributes, none of them are decimal type. @FiveDigital So don't improve it because there might be regressions? Isn't that what the exhaustive unit tests are for? |
I just wanted to point this topic out! We had so much pain with this issue in the time between 1.4 and 1.8 and I'm sure we weren't the only ones. So nobody wants this problems again. I'm of course not against the change and covering the rounding issues with unit tests is a must have even if the decimal format doesn't change 👍 |
@FiveDigital agreed. It is also really hard to cover all possible cases with tests (quite a lot of possibilities incl. price rules, shipping, coupons, promos, gift cards etc) so a change in this area can introduce undetected errors. |
Rounding errors are far from solved. The minute you set up taxes, a Decimal precision has already been discussed on a different thread. My So use as many decimal places as possible to store prices and remove the Use cases are plenty:
All of the above are present in most B2B stores I have dealt with.
|
Invoices from telecommunication providers often show 4 or six digits On 8/23/14, Barbazul [email protected] wrote:
|
As @ilol said, the IRR exchange rate in Iran for EURO is "1 IRR = 0.0000283999 EUR" and for USD is "1 IRR = 0.0000376100 USD" ... So I think even (20,10) will make sense for Iranians |
@barbazul You seem to be equating storage with calculations.. Magento does all calculations in the PHP code which has no bearing whatsoever on storage. Only if Magento was using SQL queries with multiplications and divisions would the number of decimal places need to support more than the final result that is stored. @DavidBruchmann Magento is not designed for telecoms and has never been and almost certainly will never be used by telecoms for usage billing. I will concede this point if a real, common case is given whereby merchants make sales in fractions smaller than their currency physically allows as per the ISO standards. Supporting cases like this when there is no good reason to at the detriment of all other merchants doesn't make sense to me, especially when the issue can be worked around easily enough by those with special needs (mysqldump | sed | mysql). |
@colin Mollenhour |
Collin: those are not separate issues. For instance, sales tables store all As for the requested example, check out www.maxiconsumo.com which is a B2B
|
If you configure your products with prices excluding taxes, you need to be able to define them with 4 decimals to get a price including taxes on 2 decimals. Example: |
@acharrex I see your point, perhaps, but your example is flawed because 115.880 * 1.08 still rounds to 125.150. :) |
@colin, you always find arguments for avoiding changes concerning the On Tue, Sep 9, 2014 at 9:09 PM, Colin Mollenhour [email protected]
|
@DavidBruchmann , @barbazul , @FiveDigital, @tzyganu , @colinmollenhour from my perspective it sounds like there are several inter-related threads:
For item 1 please note we need to store more details for orders to properly calculate sales/shipping tax. That work might impact how many digits we have to store ( issues #444 ). As @ilol stated we'll look into this but it's tied up with the other item's which compound the matter. For item 2 - currency conversion. Isn't that just saying we should store a # of significant digits for currency conversation ratios (i.e. just 1 table; for the most part)? This seems pretty reasonable to adjust For item 3 B2C - we really only support 2 digits of precision; so per ISO 4217 we're really not supporting those currencies using 3 or 4 digits of precision (i.e. COU, LYD, XBT [ I thought there was a bitcoin craze going on... heck there's 8 digits there ] ; of which majority of currencies are 2 digits of precision ). To support this we'd need to enable a configuration for merchants to change from say 2 digits to 8 digits (bit coin; 4 if we stick to non-digital currencies) of precision and show that precision on all prices in Magento. This would also impact storage and tax calculations. This seems pretty risk for what is a handful of countries with a small share of the e-commerce market. Which e-commerce sites are located/used in Tunisia, Oman, Libya, Colombia...? If one were to customize this behavior is it do able given the price template work and consolidated tax logic? What else would need to be done to help support community efforts here? For item 3 B2B support - that tends to look more like digits of precision for a product * some amount/rounded to the currency precision as a line total. This should be easier to customize with the recent price template refactoring and consolidated tax calculation logic work. At present our focus is B2C. For item 4 - @FiveDigital is totally right; I wouldn't feel comfortable making a change for item 3 until we've done more work around automated testing ( considerably more work ). |
Regarding item 3. I don't know about many of those countries but I have worked in 2 Magento Also, they have an annual ecommerce day conference that.claims to be the So i'd dare say Colombia has a significant ecommerce activity. More to the precision matter: So again, I will have to hack my way around all those hardcoded
|
I'm putting in my $0.02000 here because I am currently faced with implementing 5 digit precision in a Magento 1.9 build. For those who believe there are no good use cases for this, they are ignoring real-world businesses that expect this sort of precision. "Our old system allowed us to do this!" As previously pointed out, consider the site that sells things in large quantity where each unit has a small price. Nails, screws, etc were mentioned, but also think about pricing per linear inch / square inch / etc. What I need currently amounts to:
TL;DR - Merchants need to be able to decide how many digits of precision they will use. In the case I provided, we need 5 (maybe even 6 for some products). I agree that merchants who only require 2 digits shouldn't have to bear the brunt of the extra precision. Maybe the best solution is to provide an option during installation where pricing precision can be chosen by the admin/developer/merchant. |
@pljspahn Thanks for the input, that is a good scenario to consider. Some thoughts on the matter: Magento does support decimal quantities, so using larger units and decimal quantities could be a solution that would work even with current limitations. That is if units are currently square inches, store prices in price per square feet or per 100 square inches. This might make sense anyway since as a customer $3.06/sqft is arguably more readable than $0.02125/sqin. Then if they order 36" x 30" that would simply be 3' x 2.5' = 7.5 units so the example problem would be supported just fine by existing limitations. Template updates can always be used to convert to smaller units if necessary. Just a thought. As I said before, even if the merchant absolutely must store the product price using higher precision, the order totals and similar do not need higher precision since they will be rounded to exchangeable monetary amounts and they account for probably 90% of the decimal columns in the database. I agree that having the merchant choose precision configuration upon installation would be a good solution. Another possibility that may be more EAV friendly would be to take the middle-ground and support two decimal types: low precision and high precision that are either fixed or configurable. Low precision would be for monetary amounts and high for things like tiny unit prices or currency exchange rates |
Configuration of high or low precision had to be done on base of the On Sat, Dec 6, 2014 at 12:26 AM, Colin Mollenhour [email protected]
|
@pljspahn @colinmollenhour @DavidBruchmann Currently I'm tracking two feature requests (in our backlog; TBD) - Bulk Pricing, and controlling degrees of pricing precision. For the latter all the permutations concern me - I'd think it'd be safer to just configure pricing/rounding for 0 or 2 or 4 digits (maybe add 3 for the middle east countries depending on testing cost). @barbazul did you create automated tests - could you share those? |
Internal Ticket ID: MAGETWO-27587 |
Update on this issue:
Asks:
Thanks! |
I know this is maybe the wrong place, but I think I have a solution for the tax/discound rounding issues. So, in my understanding the problem is the way magento calculates discounts, tax and totals. It calculates them all separately and also rounds them separately which is wrong. Example: Item price: 4,30 € Magento does: Magento displays both sums rounded up, which causes: What should have happened is: Item price: 4,30 € What I did to "solve" this: I changed the column definitions from the following fields from DECIMAL(12,4) to DECIMAL(12,2) sales_flat_order_item.discount_amount sales_flat_quote_item.discount_amount This forces magento to use an already rounded discount/tax to calculate the final price. I know its not ideal, but for the current circumstances I deem this a way to do it. This only works, if your tax setting is set to have taxes already included in the product price (which it is in our usecase). Not really sure how much impact that is going to have on the rest of the shop - my initial tests showed good results. More testing will happen tomorrow with different discounts (sales_rules as well) and different tax settings (values and display settings). I would really like if the calculation for the final discounted and taxed prices would be done properly in magento2 - like I described above. Rounding first on the discounts and then calculating the final price from that instead of the way this is done now. Cheers! |
@kortwotze Thanks! I'll pass this insight along to our Tax Architect @tang-yu and have him look at it. Likely he won't be able to provide feedback on this for a few weeks. We had a similar issue with discounts that was found earlier in our develop branch for Magento 2 and I believe corrected. Are you still seeing your example on a recent Magento 2 develop branch site? |
To be perfectly honest - as I stated in the beginning: This might be in the wrong place, since I am using Magento 1.9.2.0 for that and my research for that brought me to my conclusion above. I thought, you might be able to use that in Mage2, because in Mage1 its still really messed up in that way. I looked into Mage2 just because I thought I´d find another approach to solve this issue. Then I found this thread and saw, there was still discussion on how to store decimals (which decimals and taxes are) and I found my "solution" beeing close to this topic (though for a different Magento, I agree fully). So, if this does not belong here, please redirect me to a proper place - otherwise, please feel free to tell me, what you think :) I did some other tweaks that might contribute to finally fixing rounding issues on taxes and discounts in Mage1 which I will write about soon(tm). |
@kortwotze Cool. We rewrote the tax engine in Magento 2 and have some minor functional differences. What you're describing in Magento 1 is a recently identified issue ( discount rounding issues when interacting with other rounding points - item tax, shipping, FPT ) and has a slightly different fix in M1 code which mirrors our engine changes in M2 ( we carry both - and + delta-rounds rather than only + ones ). We're making some tweaks to our M2 tax engine to address them and will effectively be porting those fixes to a future M1x release. @piotrekkaminski is aware of this work and tracking in our M1.x backlog. |
@kortwotze , I agree with you that most of the rounding issue was caused by rounding prices separately and then adding prices together. We changed the calculation logic so that we round tax or discount first and subtract tax to get price excluding tax. We did this in the code, e.g., https://github.com/magento/magento2/blob/develop/app/code/Magento/Tax/Model/Calculation/UnitBaseCalculator.php#L56 and https://github.com/magento/magento2/blob/develop/app/code/Magento/Tax/Model/Calculation/UnitBaseCalculator.php#L81. |
@tang-yu @choukalos Thank you very much for this input. I am very glad to see the way, Mage2 is handling this (I did not look into this until now, because we are not bothering with Mage2 for now). The main question right now is: When will this backport fix be released for Magento 1.9.x.x? I dont really want to go live with those database hacks for our new project, but we have a deadline and I will use my approach to secure that deadline. :sadface: Anyway, thanks a lot for this input :) I was very scared of rewriting Mage_Tax and Mage_Sales for that, because it is sooo much. :D |
@kortwotze we have plans to improve this in 1.x but it will take time - the next bugfix release is expected next year. |
Thanks for the reply. Though its sad, that the fix is expected next year, it is good, that the fix eventually will be released. I really appreciate it :) |
After a complete year away from the M2 project I am finally forced to get back to it due to its recent release and @benmarks fundamentals course that I am currently taking. I'll play around with M2 a little bit longer, getting the hang of it and then I'll send my PR (if this ihas not been done already by someone else) |
Thank you for your submission. We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues. Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here). We are closing this GitHub ticket and have moved your request to the new forum. |
@magento-engcom-team Is this scheduled to be in in 2.3? |
@ctadlock @magento-engcom-team |
Imo we should keep 4 as many shops need this precision. Don't change this. |
this is not usable if you want add BTC as Currency to shop. also hardocded precision = 2
|
You can apply patches if you need a different logic. |
MySQL decimal storage uses a minimum of 4 byes and can store 9 digits in these 4 bytes. Going up to 6 bytes you can store 13 digits.
In most cases decimal(9,3) is sufficient and it would save 2 bytes per row/column. The maximum value would be 999,999.999 (1 million) instead of 99,999,999.9999 (100 million). Unless you are selling original artwork in Iranian Rial this should be sufficient. Also according to the ISO list of currencies there are no currencies which use 4 decimal places (but plenty that use 3, so 2 is not acceptable).
For aggregation tables the better storage type would be decimal (13,3) since still 4 decimals are not needed and 13 takes the same storage space as 12. So for the same storage you can store numbers 100 times larger.
It may seem like a minor optimization but in Magento 1 my sales_* tables currently have over 500 decimal(12,4) columns (some are third-party). 2 bytes * 500 columns * 1 million rows is almost 1GB of storage, not to mention the indexed columns. Not huge, but quite significant.
While you're at it, the report_event_types.event_type_id and report_event.event_type_id and report_event.subtype should be tinyint instead of smallint. Maybe these were removed or already changed in M2.. There are probably other columns that could be optimized as well. I think there is even a strong case to be made for changing store_id to "tinyint unsigned" throughout since operating over 256 stores is an edge case.
I realize the DBMS abstraction may be an impediment to this change but considering only MySQL is actually supported I think that is far less important than a database that isn't wasteful of resources.
Lastly, for anyone who is an edge case and decimal(9,3) is not sufficient it should be quite easy to install Magento, dump the database, use sed or a text editor to do a simple replacement and then re-import.
The text was updated successfully, but these errors were encountered: