Repository navigation
Fix sin cos degrees negative quotient - #53
Merged
alexey-milovidov merged 1 commit intoOct 25, 2026
Merged
alexey-milovidov merged 1 commit into
alexey-milovidov merged 1 commit into
Conversation
`sin_cos_degrees` converted the quotient to `unsigned`, but the quotient is negative for angles below -45 degrees, and that conversion is undefined behavior. x86 happens to wrap around and pick the right quadrant, while AArch64 saturates to zero, so Karney's formulas (which make the first latitude negative) returned wrong distances there, e.g. 1398247 m instead of 1223596 m for London - Paris - Berlin. Convert through `int` instead.
pulk17
marked this pull request as draft
October 25, 2026 18:22
pulk17
marked this pull request as ready for review
October 25, 2026 18:25
alexey-milovidov
approved these changes
Oct 25, 2026
alexey-milovidov
added a commit
to pulk17/ClickHouse
that referenced
this pull request
Oct 26, 2026
The pinned commit `598067791` was merged into `clickhouse-1.90.0` as `0286c8f53`; the trees are identical, so this only points the submodule at the branch head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
math::sin_cos_degrees(boost/geometry/util/math.hpp:856-887) reduces the angle to [-45, 45] and picks the quadrant withswitch (unsigned(quotient) & 3U), wherequotient = floor(fmod(x, 360) / 90 + 0.5).fmodkeeps the sign ofx, so the quotient is negative for every angle below -45 degrees, and converting a negative floating-point value tounsignedis undefined behavior. With the usual SSE conversion, x86-64 wraps around (-1.0gives0xFFFFFFFF, quadrant 3, which is right), but AArch64 saturates to 0 (quadrant 0), sosinandcoscome out for the wrong angle.Karney's formulas call it on the latitude after arranging the points so that the first latitude is negative (
formulas/karney_inverse.hpp:184-201), so on AArch64 any geodesic with a latitude beyond 45 degrees north or south was wrong. Withstrategies::length::geographic<strategy::karney>and WGS84:(-0.1276 51.5072, 2.3522 48.8566, 13.405 52.52)(-68.3 -54.8, -70.65 -33.45)(0 -60, 10 -60, 20 -75)This showed up in ClickHouse CI on Graviton, which returned exactly the first row's 1398247.153 m (ClickHouse/ClickHouse#127764, the first ClickHouse function that uses geographic coordinates). The whole AArch64 column is reproduced on x86 by replacing the conversion with a saturating one, and UBSan reports it on x86 as
math.hpp:873:22: runtime error: -1 is outside the range of representable values of type 'unsigned int'. The correct column agrees with an independent geodesic solver to the micrometer.This PR converts the quotient to
intfirst: for a finite angle it is within [-4, 4], and converting a negativeinttounsignedis well defined. For a NaN or infinite angle the quotient is NaN, so it uses quadrant 0 instead of converting it;sinandcosof the remainder are NaN there and every quadrant gives NaN, as before. On x86-64 the output is bit-identical to before for 4.17 million angles (every multiple of 0.001 in [-1080, 1080], 64 neighbouring values on each side of every multiple of 45, 2 million random angles in [-10^6, 10^6], and the special values), and UBSan reports nothing for them, while the saturating conversion of AArch64 gives a different result for 37.5% of them (the angles whose remainder is in (-315, -45)).The only callers are
karney_directandkarney_inverse, so the inverse and the direct geodesic problems are both fixed. Upstreamboostorg/geometrydevelophas the same line.