Skip to content

release-25.4: testutils/floatcmp: increase CloseMargin to handle distributed execution errors#162223

Open
blathers-crl[bot] wants to merge 1 commit intorelease-25.4from
blathers/backport-release-25.4-161907
Open

release-25.4: testutils/floatcmp: increase CloseMargin to handle distributed execution errors#162223
blathers-crl[bot] wants to merge 1 commit intorelease-25.4from
blathers/backport-release-25.4-161907

Conversation

@blathers-crl
Copy link

@blathers-crl blathers-crl bot commented Feb 2, 2026

Backport 1/1 commits from #161907 on behalf of @mw5h.


Previously, the CloseMargin constant was set to
CloseFraction * CloseFraction (1e-28), which was far too small to handle real-world floating-point rounding errors that occur when comparing results from different query execution paths (distributed vs local). This caused the unoptimized-query-oracle roachtest to flake when aggregate functions like stddev_pop and var_samp produced values differing by ~1e-16 due to different aggregation ordering.

Additionally, the test only used approximate float matching (FloatsMatchApprox) on s390x architecture, while using exact significant-digit matching on other platforms, which didn't leverage the CloseMargin tolerance at all.

This commit makes two changes:

  1. Increases CloseMargin from CloseFraction * CloseFraction (1e-28) to CloseFraction / 10 (1e-15), which is appropriate for catching the numerical noise observed in distributed aggregate computations.

  2. Changes unsortedMatricesDiffWithFloatComp to always use FloatsMatchApprox for float comparisons on all architectures, not just s390x. This ensures the CloseMargin tolerance is consistently applied.

Fixes #150902

Release note: none

Epic: None


Release justification:

…ion errors

Previously, the `CloseMargin` constant was set to
`CloseFraction * CloseFraction` (1e-28), which was far too small to
handle real-world floating-point rounding errors that occur when
comparing results from different query execution paths (distributed vs
local). This caused the `unoptimized-query-oracle` roachtest to flake
when aggregate functions like `stddev_pop` and `var_samp` produced
values differing by ~1e-16 due to different aggregation ordering.

Additionally, the test only used approximate float matching
(`FloatsMatchApprox`) on s390x architecture, while using exact
significant-digit matching on other platforms, which didn't leverage
the `CloseMargin` tolerance at all.

This commit makes two changes:

1. Increases `CloseMargin` from `CloseFraction * CloseFraction` (1e-28)
   to `CloseFraction / 10` (1e-15), which is appropriate for catching
   the numerical noise observed in distributed aggregate computations.

2. Changes `unsortedMatricesDiffWithFloatComp` to always use
   `FloatsMatchApprox` for float comparisons on all architectures, not
   just s390x. This ensures the `CloseMargin` tolerance is consistently
   applied.

Fixes #150902

Release note: none

Epic: None

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-25.4-161907 branch from b2489bc to 7f3c2a7 Compare February 2, 2026 17:15
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Feb 2, 2026
@blathers-crl blathers-crl bot requested a review from yuzefovich February 2, 2026 17:15
@blathers-crl
Copy link
Author

blathers-crl bot commented Feb 2, 2026

Thanks for opening a backport.

Before merging, please confirm that it falls into one of the following categories (select one):

  • Non-production code changes. Includes test-only changes, build system changes, etc.
  • Fixes for serious issues. Defined in the policy as correctness, stability, or security issues, data corruption/loss, significant performance regressions, breaking working and widely used functionality, or an inability to detect and debug production issues.
  • Other approved changes. These changes must be gated behind a disabled-by-default feature flag unless there is a strong justification not to.

Add a brief release justification to the PR description explaining your selection.

Also, confirm that the change does not break backward compatibility and complies with all aspects of the backport policy.

All backports must be reviewed by the TL and EM for the owning area.

@trunk-io
Copy link
Contributor

trunk-io bot commented Feb 2, 2026

Merging to release-25.4 in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

@blathers-crl blathers-crl bot added backport Label PR's that are backports to older release branches T-sql-queries SQL Queries Team labels Feb 2, 2026
@blathers-crl
Copy link
Author

blathers-crl bot commented Feb 2, 2026

✅ PR #162223 is compliant with backport policy

Confidence: high
Backward compatible: true
Explanation: The pull request involves modifications to files that are exclusively within the scope of test utilities (testutils) and testing-related changes in roachtest/tests. According to the CockroachDB backport policy, changes affecting non-production code, such as testing infrastructure, are exempt from the requirements to fix a critical bug or need to be gated behind a feature flag. Since the affected files (floatcmp.go, floatcmp_test.go, and query_comparison_util.go) fall under test utilities, which are considered non-production, the pull request is compliant without needing further justification via 'Release justification' or criteria for critical bugs.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. T-sql-queries SQL Queries Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants