<div dir="ltr">+1 to correctly respecting the limit on Max DECIMAL Digits<div><br><div>-2 to changing the semantics to Max TOTAL Digits</div><div><br></div><div>Reasons:</div><div>- The current documentation and design intent is to provide the ability to specify Max DECIMAL Digits, so changing this would be a breaking change</div></div><div>- It is not useful to specify the Max TOTAL Digits, since without knowing the magnitude of the numbers this leads to unknown and possibly varying precision of output numbers</div><div>- All text formats are designed to easily accommodate varying-length numbers, so there is no reason to specify the length of numeric strings (Max TOTAL Digits). </div><div>- Whereas there is a clear utility in specifying Max DECIMAL Digits</div><div><br></div><div>It's unfortunate if implementing true Max DECIMAL Digits requires more code & test changes than the alternative, but the most important thing is to provide stable and useful semantics for users going forward.</div><div><br></div><div>Martin </div><div><br></div><div>  </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Apr 10, 2020 at 9:33 AM <<a href="mailto:rmrodriguez@carto.com">rmrodriguez@carto.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">PRs with the changes (there are also some changes in the cunit<br>
structure to ease the diff showing):<br>
<br>
- Enforcing that we output the amount of DECIMAL digits that the user<br>
has requested as precision:<br>
<a href="https://github.com/postgis/postgis/pull/554" rel="noreferrer" target="_blank">https://github.com/postgis/postgis/pull/554</a><br>
- Consider the precision as number of TOTAL (integer + decimal)<br>
digits: <a href="https://github.com/postgis/postgis/pull/555" rel="noreferrer" target="_blank">https://github.com/postgis/postgis/pull/555</a><br>
<br>
The first one does exactly what's documented in the functions<br>
themselves, that is if you request precision you get<br>
1.111111100000000018 (before you got 1.1111111), but the diff is<br>
really massive (2435<br>
changes just in regress/). The second one is closer to the current<br>
behaviour (35 changes in regress/).<br>
<br>
In light of these tests, I propose changing the documentation to<br>
declare the precision as the number of total digits (before and after<br>
the point) and enforce that for all functions for 3.1+, i.e. this PR<br>
(<a href="https://github.com/postgis/postgis/pull/555" rel="noreferrer" target="_blank">https://github.com/postgis/postgis/pull/555</a>) plus documentation<br>
changes.<br>
<br>
On Fri, Apr 10, 2020 at 4:22 PM <<a href="mailto:rmrodriguez@carto.com" target="_blank">rmrodriguez@carto.com</a>> wrote:<br>
><br>
> Also note that it would be possible to limit the number of digits<br>
> (integer or decimal) instead, which could be preferable but that's a<br>
> breaking change to all the extension functions (since they are<br>
> requesting 15 decimal digits, not 15 total digits).<br>
><br>
> On Fri, Apr 10, 2020 at 4:07 PM <<a href="mailto:rmrodriguez@carto.com" target="_blank">rmrodriguez@carto.com</a>> wrote:<br>
> ><br>
> > Hi everyone,<br>
> ><br>
> > Lately I've been trying to improve the performance of output functions<br>
> > and one of the areas where I got a massive (5x) improvement was<br>
> > anything that output text (ST_AsText, ST_AsGeoJSON and so on) but the<br>
> > implementation that I introduced for 3.1 has multiple hacks to keep<br>
> > the output almost exactly the same as it was for 3.0.<br>
> ><br>
> > Although most output functions have a `maxdecimaldigits` parameter<br>
> > that represents `maximum number of decimal digits after floating<br>
> > point` there are multiple cases where this isn't respected. Some<br>
> > examples:<br>
> ><br>
> > ```<br>
> > Select ST_AsText(ST_MakePoint(123456789012.1234567890123, 0), 4);<br>
> > POINT(123456789012.123459 0)<br>
> > ```<br>
> > The number should have 4 digits, but has 6.<br>
> ><br>
> > ```<br>
> > Select ST_AsText(ST_MakePoint(0, 92114.013999999996), 15);<br>
> > POINT(0 92114.014)<br>
> ><br>
> > Select ST_AsText(ST_MakePoint(0, 92114.013999999996), 20);<br>
> > POINT(0 92114.013999999995576)<br>
> > ```<br>
> ><br>
> > This number has a significant digit on the 12th decimal digits, but<br>
> > it's not shown if you request 15 decimal digits. But it is shown if<br>
> > you request 20 decimal digits.<br>
> ><br>
> > On the other hand, it doesn't work with ST_AsGeoJSON because it's<br>
> > limited internally:<br>
> > ```<br>
> > Select ST_AsGeoJSON(ST_MakePoint(0, 92114.013999999996), 20);<br>
> > {"type":"Point","coordinates":[0,92114.014]}<br>
> > ```<br>
> ><br>
> > So I want to propose some changes to unify the behaviour of all<br>
> > functions that output coordinates. They aren't big changes but the<br>
> > output of multiple regression tests will change, thus 3.0 and 3.1<br>
> > output will change in some cases too; but I think it's worth it as we<br>
> > get rid of bugs and we uniformize the output of all functions.<br>
> ><br>
> > The rules would be like this:<br>
> > - For (absolute) values under FP_TOLERANCE, we keep returning '0'.<br>
> > - For big numbers (absolute value bigger than 1E15), we keep the<br>
> > current behaviour and maintain the decimal digits to 8 - length of the<br>
> > decimal part (it's odd but I don't see the need to change it).<br>
> > - For smaller numbers:<br>
> >   - The integer part is always printed in full (no changes).<br>
> >   - The decimal part is always printed up to precision digits<br>
> > (removing trailing zeros).<br>
> >   - The decimal digit precision is capped between 0 and 20. This is<br>
> > more than enough to guarantee round trip (double -> text -> same<br>
> > double) and it allows us to know the max size of a double (so we can<br>
> > keep the static allocations) as sign + 15 integer digits + "." + 20<br>
> > decimal digits + "NULL".<br>
> ><br>
> > I'm going to draft a PR with the changes, but any comment is more than<br>
> > welcome before I push a change like this.<br>
> ><br>
> > --<br>
> > Raúl Marín Rodríguez<br>
> > <a href="http://carto.com" rel="noreferrer" target="_blank">carto.com</a><br>
><br>
><br>
><br>
> --<br>
> Raúl Marín Rodríguez<br>
> <a href="http://carto.com" rel="noreferrer" target="_blank">carto.com</a><br>
<br>
<br>
<br>
-- <br>
Raúl Marín Rodríguez<br>
<a href="http://carto.com" rel="noreferrer" target="_blank">carto.com</a><br>
_______________________________________________<br>
postgis-devel mailing list<br>
<a href="mailto:postgis-devel@lists.osgeo.org" target="_blank">postgis-devel@lists.osgeo.org</a><br>
<a href="https://lists.osgeo.org/mailman/listinfo/postgis-devel" rel="noreferrer" target="_blank">https://lists.osgeo.org/mailman/listinfo/postgis-devel</a></blockquote></div>