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

Fix output precision limit for double values (issue #118) #119

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

vitcpp
Copy link
Contributor

@vitcpp vitcpp commented Mar 14, 2024

pgSphere used its own setting (set_sphere_output_precision function) to limit the output precision of double values, that could not be greater than 15 significant digits (DBL_DIG). It introduced some problems with dump/restore. PostgreSQL uses its own precision setting: extra_float_digits. The PostgreSQL setting allows to use more significant digits.

This patch changes the default pgSphere output behaviour to utilize PostgreSQL extra_float_digits. Now, extra_float_digits is used by default, until set_sphere_output_precision is called.

The old behaviour is kept for compatibility purposes. Once, set_sphere_output_precision is called, pgSphere starts to use the old behaviour (read, please, issue #118 discussion).

The patch introduces a new function - reset_sphere_output_precision. It is used to reset to the PostgreSQL behaviour after using set_sphere_output_precision.

pgSphere used its own setting (set_sphere_output_precision function)
to limit the output precision of double values, that could not be
greater than 15 significant digits (DBL_DIG). It introduced some
problems with dump/restore. PostgreSQL uses its own precision setting:
extra_float_digits. The PostgreSQL setting allows to use more significant
digits.

This patch changes the default pgSphere output behaviour to utilize
PostgreSQL extra_float_digits. Now, extra_float_digits is used by default,
until set_sphere_output_precision is called.

The old behaviour is kept for compatibility purposes. Once,
set_sphere_output_precision is called, pgSphere starts to use the old
behaviour (read, please, issue postgrespro#118 discussion).

The patch introduces a new function - reset_sphere_output_precision.
It is used to reset to the PostgreSQL behaviour after using
set_sphere_output_precision.
@esabol
Copy link
Contributor

esabol commented Mar 14, 2024

This is fantastic, @vitcpp!

I'm not sure I understand why there are two new expected/*.out files, but no corresponding new *.sql files to generate them. Is that intentional?

@vitcpp
Copy link
Contributor Author

vitcpp commented Mar 15, 2024

Hi @esabol, once I've changed the default behaviour of the output by improving the precision, some tests show differences in numbers. It is why I've added two new test variant comparison files. But, it seems it may be a mistake. It would be better to just replace the old files. I will check. Thank you. BTW, I think to add some a new test for testing output precision with different settings.

P.S. Test variant comparison files are used if the output may vary on different platforms: https://www.postgresql.org/docs/current/regress-variant.html

P.P.S. I'm going to check it on 32 bit platform as well.

@esabol
Copy link
Contributor

esabol commented Mar 15, 2024

The changes should definitely have tests. I was just confused as to how the new expected/*.out files did that. Variant comparison files are not something I was familiar with. Thank you for the link explaining the concept. If you think this is the best way to implement these tests, I am fine with it.

@vitcpp
Copy link
Contributor Author

vitcpp commented Mar 15, 2024

expected/epochprop.out differs for 10, 11 and for 12+ versions, because the default precision was 15 significant digits prior to PG12. It is why I've added one more variant comparison file. At the left - output for PG 10-11, at the right - output for PG 12+.

image

@df7cb
Copy link
Contributor

df7cb commented Mar 15, 2024

Why not put set extra_float_digits = 2; at the top of each "general" test file, and only check for the varying precision in a specific test for that purpose, if at all? (Ok, I see it's only the gist bounding box test that's affected, but that seems unrelated functionality.)

@vitcpp
Copy link
Contributor Author

vitcpp commented Mar 15, 2024

Why not put set extra_float_digits = 2; at the top of each "general" test file, and only check for the varying precision in a specific test for that purpose, if at all? (Ok, I see it's only the gist bounding box test that's affected, but that seems unrelated functionality.)

It makes sense, thank you! It seems, set_sphere_output_precision(8) is set at the top of almost all tests.

Copy link
Contributor

@esabol esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

@vitcpp
Copy link
Contributor Author

vitcpp commented Mar 18, 2024

I've added one more commit to set extra_float_digits = 2 in epochprop and bounding_box_gist tests as proposed by @df7cb . It allows to remove expexted/bounding_box_gist_2.out but expected/epochprop_1.out is still required. The reason to create expected/epochprop_1.out is that the earlier versions output exactly 17 digits if extra_float_digits is 2, some of these digits may be meaningless. PG versions 12+ use a special function that outputs only meaningful digits even if extra_float-digits = 2, it is why in PG 12+ some numbers may contain less than 17 digits if the text representation if enough to define the double number.

Copy link
Contributor

@esabol esabol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better!

@vitcpp vitcpp merged commit 8394ae7 into postgrespro:master Mar 26, 2024
14 checks passed
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