Fix json rendering of large osm ids #7096
Conversation
imannamdari
force-pushed
the
fix-large-osmids-json-rendering
branch
from
e5df0c4 to
ef96e49
Compare
include/util/json_renderer.hpp
Outdated
| @@ -54,7 +54,10 @@ template <typename Out> struct Renderer | |||
| // `fmt::memory_buffer` stores first 500 bytes in the object itself(i.e. on stack in this | |||
| // case) and then grows using heap if needed | |||
| fmt::memory_buffer buffer; | |||
| fmt::format_to(std::back_inserter(buffer), FMT_COMPILE("{:.10g}"), number.value); | |||
| if (static_cast<std::uint64_t>(number.value) == number.value) | |||
| fmt::format_to(std::back_inserter(buffer), FMT_COMPILE("{}"), static_cast<std::uint64_t>(number.value)); | |||
There was a problem hiding this comment.
Is the if/else block really needed? Can't a single line below handle the big integer number up to 18446744073709551615 (which is 2^64-1)?
fmt::format_to(std::back_inserter(buffer), FMT_COMPILE("{}"), static_cast<std::uint64_t>(number.value));
There was a problem hiding this comment.
Ah, nevermind, I see that it would break the fractional numbers
There was a problem hiding this comment.
Exactly. For floating-points there gonna be problems.
The previous approach implemented before this commit can be found here (and it works fine):
A more robust solution could be separating floats from integers. Instead of Number struct, we can have Float and Integer structs for example. Each with their own json rendering.
However, that's a significant change! :D
If you agree with this approach (separating floats from integers), I can work on it in another pull request.
There was a problem hiding this comment.
Dear @DennisOSRM and @SiarheiFedartsou now that CI build is working again, could you please consider merging this fix by @imannamdari ?
In the past week I have tested his fix with map matching and it really seems to fix the problem with large OSM node/way ids.
And please consider adding a new git tag v5.28.1 after the merge?
Because the current tag v5.27.1 is already three years old and it would be nice to set a newer stable source code "checkpoint" for all users.
There was a problem hiding this comment.
new release coming soon
There was a problem hiding this comment.
Hi @DennisOSRM I already run a version with this fix applied and it does fix the large OSM ids.
However the confidence is sometimes still printed as a scientific notation.
I am not sure if this is a problem or not and wonder why is json_renderer not called for the confidence.
Here the URL showing this:
There was a problem hiding this comment.
Hi @afarber,
I've tried the example you provided, and it seems to convert the confidence value into scientific notation. The confidence value appears to be this number:
0.000000000000547
This section in json_renderer seems to be responsible for converting it to scientific notation:
FMT_COMPILE("{:.10g}")
This behavior already exists, and this merge request doesn't introduce it (since we're only addressing integer number representation in this MR).
I'm not sure what the best approach here should be. We could manually set the precision to 10 digits (similar to the previous implementation) or consider another solution. My question is: is the scientific notation really a problem? Since it's a valid representation of a float/double number, most JSON parsing frameworks should handle it without issues. Are you encountering any specific problems when parsing it?
There was a problem hiding this comment.
Maybe you have different OSM data than me, I have used berlin-latest.osm.pbf
Or maybe I have misapplied your patch.
No, it parses well for me with C# System.Text.Json
There was a problem hiding this comment.
No, I get the same result as you (5.470068842e-13). The actual double value in the code is 0.000000000000547, which is represented in JSON as 5.470068842e-13.
The issue with this number is that it is too small. The previous implementation would return 0.0000000000 in the JSON response, which was not an ideal solution for representing very small floating-point numbers. As a result, the current approach was implemented to handle the rendering of extremely small float numbers more accurately (see this PR).
Therefore, I believe it is best to leave the current JSON rendering for floating-point numbers as it is and focus on editing it for integer numbers, which is the intent of this PR.
Issue
Fix #7016
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?