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

Failing test in master (json serialization) #3863

Closed
jjimenezshaw opened this issue Aug 28, 2023 · 2 comments
Closed

Failing test in master (json serialization) #3863

jjimenezshaw opened this issue Aug 28, 2023 · 2 comments
Labels

Comments

@jjimenezshaw
Copy link
Contributor

Example of problem and description

As you can see in https://github.com/OSGeo/PROJ/actions/runs/5992619233/job/16252194635#step:6:2595
there is a failing test in master: json_import.id_code_string_version_double. The same happens for some windows compilations.

Unfortunately I have only an Ubuntu 22.04 to try to find anything.

If I understand correctly, the serialization is done in this piece of code in metadata.cpp

void Identifier::_exportToJSON(JSONFormatter *formatter) const {
...
    std::string l_version = *version();
...
            try {
                const double dblVersion = c_locale_stod(l_version);
                if (dblVersion >= std::numeric_limits<int>::min() &&
                    dblVersion <= std::numeric_limits<int>::max() &&
                    static_cast<int>(dblVersion) == dblVersion) {
                    writer->Add(static_cast<int>(dblVersion));
                } else {
                    writer->Add(dblVersion);
                }
            } catch (const std::exception &) {
                writer->Add(l_version);
            }

More exactly in the line writer->Add(dblVersion);
There it calls

void CPLJSonStreamingWriter::Add(double dfVal, int nPrecision)

with the default value of nPrecision for double: 18

There it calls

CPLSPrintf(szFormatting, dfVal)

with szFormatting = "%.18g" and dfVal = 9.8d (my debugger shows 9.8000000000000007)

That finally calls sqlite3_vsnprintf in static std::string CPLSPrintf(const char *fmt, ...).

SQLite documentation https://www.sqlite.org/c3ref/mprintf.html says that it is similar to "snprintf()" from the standard C library.

This example of sqlite3 produces the expected result, 9.8. https://godbolt.org/z/4545f4vaq

But using the standard C library, it does not: https://godbolt.org/z/hd585fPzj

Reached to this point I do not know how to follow the investigation. I guess something changed in sqlite3, or any other library. (or in the method c_locale_stod, but I don't think so)

BTW, why are we serializing and "id" as a double, and not as the original string?

Expected Output

No failing tests.

Environment Information

  • PROJ version (proj): master
  • Operation System Information: MacOS and others.

Installation method

  • GitHub actions
@rouault
Copy link
Member

rouault commented Aug 28, 2023

fix in #3864 by reducing the number of significant digits.

BTW, why are we serializing and "id" as a double, and not as the original string?

The WKT representation allows both a number (integer or floating-point) or a string: | ::= | |
In the model, we don't keep which data type in comes from and just store a string (dequoting the original quoted string if it was). So we have to use a heuristics when exporting to figure out whether it looks like a number of a string.

rouault added a commit to rouault/PROJ that referenced this issue Aug 28, 2023
… id.version field (fixes OSGeo#3863, reworks previous commit)
@jjimenezshaw
Copy link
Contributor Author

Thanks for the fix, @rouault
The PR makes sense.

rouault added a commit that referenced this issue Aug 28, 2023
JSON export: avoid non-significant decimal digits in version field (fixes #3863)
rouault added a commit that referenced this issue Aug 28, 2023
… id.version field (fixes #3863, reworks previous commit)
rouault added a commit that referenced this issue Aug 29, 2023
[Backport 9.3] JSON export: avoid non-significant decimal digits in version field (fixes #3863)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants