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

logictest: compare floating point values approximately on s390x #1

Closed
wants to merge 1 commit into from

Conversation

jonathan-albrecht-ibm
Copy link

Overview

On s390x in the std math package and some c-deps, floating point calculations can produce results that differ from the values calculated on amd64. This patch adds a function to compare logictest floating point and decimal values within a small relative margin on s390x. The existing behavior on all other platforms remains the same.

On s390x, there are three main reasons that floating point calculations sometimes give different results:

  • the go compiler generates the s390x "fused multiply and add" (FMA) instruction where possible,
  • the go math package uses s390x optimized versions of some functions,
  • some c libs eg. libgeos, libproj also have platform specific floating point calculation differences.

Proposal

The motivation for this work is so that users building CRDB on s390x do not need to diagnose tests that fail because of platform dependent floating point differences.

This PR proposes one possible approach to dealing with platform dependent floating point differences. Since development, testing and CI are done on amd64 it keeps the current logic for determining float equality exactly the same. On s390x, it determines values of decimal and float column types (R and F) in query tests to be equal if they are within a tolerance. See the new pkg/testutils/floatcmp package for the implementation of the approximate equality logic and changes in logictest.go to see how it is applied to only s390x.

There are probably other approaches I haven't thought of that would also work. I'd like to use this proposal to start a conversation on how all tests in CRDB that currently fail due to expected floating point differences could eventually be made to pass.

Of course platforms other than s390x may also have differences but I haven't looked at any other platforms. The changes should be easily extendable to other platforms if needed.

Future Work

The changes in this PR allow the following tests to pass on s390x:

  • TestLogic/fakedist-disk/builtin_function/extra_float_digits_3
  • TestLogic/fakedist-metadata/builtin_function/extra_float_digits_3
  • TestLogic/fakedist-vec-off/builtin_function/extra_float_digits_3
  • TestLogic/fakedist/builtin_function/extra_float_digits_3
  • TestLogic/local-spec-planning/builtin_function/extra_float_digits_3
  • TestLogic/local-vec-off/builtin_function/extra_float_digits_3
  • TestLogic/local/builtin_function/extra_float_digits_3

There are about 70 more tests that currently fail due to platform floating point differences on s390x, many are tests of geospatial functions. Assuming we can come up with a good approach, I'd like to continue working on fixes to be submitted in future PRs.

Release note: None

On s390x in the std math package and some c-deps, floating point
calculations can produce results that differ from the values calculated
on amd64. This patch adds a function to compare logictest floating point
and decimal values within a small relative margin on s390x.

Release note: None
@jonathan-albrecht-ibm jonathan-albrecht-ibm force-pushed the floating_point_test_tolerance branch from c351926 to adc2efc Compare April 7, 2021 18:05
@jonathan-albrecht-ibm
Copy link
Author

Closing since not needed now that cockroachdb#63244 has been raised

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.

1 participant