dividing money by money
in [PgSql]
|
From: "Kevin Grittner" on 1 Jun 2010 11:45 Alvaro Herrera <alvherre(a)commandprompt.com> wrote: > Hmm, cvs diff -Ncp does show method names too, so this is probably > filterdiff removing them. My bad; I apparently got confused somehow when looking at a context diff -- the function names are indeed there after the filterdiff conversion. Sorry for the noise on that. > BTW maybe the developer faq could use all the info gathered in > this thread. I'll take a look at that today. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: "Kevin Grittner" on 21 Jun 2010 14:47 Andy Balholm <andy(a)balholm.com> wrote: > On May 30, 2010, at 6:53 AM, Kevin Grittner wrote: >> You would then generate a diff in context format and post to the >> -hackers list with that file as an attachment. > > Here it is ================= Submission review ================= * Is the patch in context diff format? Yes, although the line endings are Windows format (CR/LF). The patch utility on my system just ignored the CRs, but if they can be filtered, all the better. * Does it apply cleanly to the current CVS HEAD? It does. * Does it include reasonable tests, necessary doc patches, etc? The doc patches seemed reasonable to me. There were no test patches; I'm not sure if they're necessary. ================ Usability review ================ ** Read what the patch is supposed to do, and consider: * Does the patch actually implement that? Yes. * Do we want that? I think we do -- it allows easy casting between money and numeric, and allows one number to be divided by another to get a ratio. * Do we already have it? There are work-arounds, but they are clumsy and error-prone. * Does it follow SQL spec, or the community-agreed behavior? There was discussion on the lists, and this patch implements the consensus, as far as I can determine. * Does it include pg_dump support (if applicable)? Not applicable. * Are there dangers? None that I can see. * Have all the bases been covered? The only possible issue is that cast from numeric to money lets overflow be noticed and handled by the numeric_int8 function, which puts out an error message on overflow which might be confusing (ERROR: bigint out of range). ============ Feature test ============ ** Apply the patch, compile it and test: * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? Just the content of the error message on the cast from numeric to money (see above). I'm not sure whether it's worth addressing that since the money class silently yields the wrong value everywhere else. For example, if you cast the numeric to text and then cast it to money, you'll quietly get the wrong amount rather than an error -- the behavior of this patch on the cast from numeric seem like an improvement compared to that; perhaps we should create a TODO entry to include overflow checking with reasonable errors in *all* money functions? Alternatively, we could modify this cast to behave the same as the cast from text, but that hardly seems like an improvement. * Are there any assertion failures or crashes? No. ================== Performance review ================== * Does the patch slow down simple tests? No. It seems to provide a very slight performance improvement for the tests run. For example, a loop through a million casts of a money literal to text runs about 1% slower than a cast of the same money literal to numeric and then to text; which is reasonable because it avoids the need to insert commas and a dollar sign. Given the number of tests, there's maybe a 10% chance that the apparent slight improvement was just noise, but given the nature of the patch, it seems reasonable to expect that there would be a slight improvement. * If it claims to improve performance, does it? It makes no such claim. * Does it slow down other things? No. ============= Coding review ============= ** Read the changes to the code in detail and consider: * Does it follow the project coding guidelines? The only issue is with the general guideline to make the new code blend in with existing code: http://wiki.postgresql.org/wiki/Submitting_a_Patch | Generally, try to blend in with the surrounding code. | Comments are for clarification not for delineating your code from | the surroundings. There are comments to set off the new code, and some of the new DATA lines (and similar) are separated away from where one would expect them to be if they had been included with the rest. Moving a few lines and deleting a few comment lines would resolve it. * Are there portability issues? I don't think so. * Will it work on Windows/BSD etc? I think so. * Are the comments sufficient and accurate? They seem so to me. * Does it do what it says, correctly? It looks like it both in reading the code and in testing. * Does it produce compiler warnings? No. * Can you make it crash? No. =================== Architecture review =================== ** Consider the changes to the code in the context of the project as ** a whole: * Is everything done in a way that fits together coherently with * other features/modules? Yes. * Are there interdependencies that can cause problems? No. ============= Review review ============= ** Did the reviewer cover all the things that kind of reviewer is ** supposed to do? I think so. I'm going to set this back to "Waiting on Author" for the minor rearrangement suggested in "Coding review". I welcome any comments from the community on the issue of the error message generated on cast from numeric to money with an out-of-bounds value. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andy Balholm on 21 Jun 2010 17:07 On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote: > Yes, although the line endings are Windows format (CR/LF). The line endings must have gotten changed in transit. My original diff used just LF. I made it on a Mac. > The only issue is with the general guideline to make the new code > blend in with existing code: > > http://wiki.postgresql.org/wiki/Submitting_a_Patch > > | Generally, try to blend in with the surrounding code. > > | Comments are for clarification not for delineating your code from > | the surroundings. > > There are comments to set off the new code, and some of the new DATA > lines (and similar) are separated away from where one would expect > them to be if they had been included with the rest. Moving a few > lines and deleting a few comment lines would resolve it. I deleted the excess comments and moved some lines around. Here it is with the changes.
From: "Kevin Grittner" on 23 Jun 2010 12:45 Andy Balholm <andy(a)balholm.com> wrote: > On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote: >> The only issue is with the general guideline to make the new code >> blend in with existing code: > I deleted the excess comments and moved some lines around. Here it > is with the changes. I ran pgindent to standardize the white space. (No changes of substance.) Patch attached. I'll mark it "Ready for Committer". Thanks, Andy! Note to committer: I'm not set up to generate docs, so I just eyeballed the sgml. -Kevin
From: Tom Lane on 15 Jul 2010 22:25
"Kevin Grittner" <Kevin.Grittner(a)wicourts.gov> writes: > Andy Balholm <andy(a)balholm.com> wrote: >> On Jun 21, 2010, at 11:47 AM, Kevin Grittner wrote: >> I deleted the excess comments and moved some lines around. Here it >> is with the changes. > I ran pgindent to standardize the white space. (No changes of > substance.) Patch attached. > I'll mark it "Ready for Committer". Applied with some editorial changes. The noncosmetic changes were: * I didn't like this bit in cash_numeric(): result->n_sign_dscale = NUMERIC_SIGN(result) | fpoint; Not only is that unwarranted chumminess with the implementation of numeric, it's flat-out wrong. If the result isn't exactly the right number of digits (say, it's 12.33999999 instead of the desired 12.34) this just hides the extra digits, it doesn't make the result correct. The right way is to use numeric_round(), which not only sets the dscale where we want it but rounds off any inaccuracy that might have crept in from the division. * The cast functions were marked immutable, which is wrong because they depend on the setting of lc_monetary. The right marking is "stable". It struck me in connection with the latter that cash_in and cash_out were also improperly marked as immutable --- they also depend on lc_monetary and so can be no better than stable. I changed that in this commit. I'm not sure if it's worth trying to back-patch; in practice people probably aren't changing lc_monetary on the fly, and the volatility of I/O functions is usually not that interesting anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |