Skip to content

Fix sin cos degrees negative quotient - #53

Merged
alexey-milovidov merged 1 commit into
ClickHouse:clickhouse-1.90.0from
pulk17:fix-sin-cos-degrees-negative-quotient
Oct 25, 2026
Merged

alexey-milovidov merged 1 commit into
ClickHouse:clickhouse-1.90.0from
pulk17:fix-sin-cos-degrees-negative-quotient

Conversation

@pulk17

@pulk17 pulk17 commented Oct 25, 2026

Copy link
Copy Markdown

math::sin_cos_degrees (boost/geometry/util/math.hpp:856-887) reduces the angle to [-45, 45] and picks the quadrant with switch (unsigned(quotient) & 3U), where quotient = floor(fmod(x, 360) / 90 + 0.5). fmod keeps the sign of x, so the quotient is negative for every angle below -45 degrees, and converting a negative floating-point value to unsigned is undefined behavior. With the usual SSE conversion, x86-64 wraps around (-1.0 gives 0xFFFFFFFF, quadrant 3, which is right), but AArch64 saturates to 0 (quadrant 0), so sin and cos come 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. With strategies::length::geographic<strategy::karney> and WGS84:

line correct AArch64 before this PR
London - Paris - Berlin (-0.1276 51.5072, 2.3522 48.8566, 13.405 52.52) 1223596.207 m 1398247.153 m
Ushuaia - Santiago (-68.3 -54.8, -70.65 -33.45) 2379420.904 m 19991386.495 m
(0 -60, 10 -60, 20 -75) 2278295.539 m 1947415.085 m

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 int first: for a finite angle it is within [-4, 4], and converting a negative int to unsigned is well defined. For a NaN or infinite angle the quotient is NaN, so it uses quadrant 0 instead of converting it; sin and cos of 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_direct and karney_inverse, so the inverse and the direct geodesic problems are both fixed. Upstream boostorg/geometry develop has the same line.

`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
pulk17 marked this pull request as draft October 25, 2026 18:22
@pulk17
pulk17 changed the base branch from master to clickhouse-1.90.0 October 25, 2026 18:25
@pulk17
pulk17 marked this pull request as ready for review October 25, 2026 18:25
@alexey-milovidov alexey-milovidov self-assigned this Oct 25, 2026
@alexey-milovidov
alexey-milovidov merged commit 0286c8f into ClickHouse:clickhouse-1.90.0 Oct 25, 2026
@pulk17
pulk17 deleted the fix-sin-cos-degrees-negative-quotient branch October 26, 2026 03:09
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>
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.

2 participants