Skip to content

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jun 27, 2024

Some assertions with floating-point values are failing on serverless because we run tests with three shards with serverless. This can cause variations in precision because data may arrive in different orders. For example, sum([a, b, c]) can yield a different precision than sum([a, c, b]). This change introduces tolerance for precision differences beyond e-10, which should be acceptable for ESQL.

@dnhatn dnhatn added >test Issues or PRs that are addressing/adding tests v8.14.3 auto-backport-and-merge :Analytics/ES|QL AKA ESQL labels Jun 27, 2024
@dnhatn dnhatn requested review from not-napoleon and nik9000 June 27, 2024 17:05
@dnhatn dnhatn marked this pull request as ready for review June 27, 2024 17:05
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 27, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we opt into this behavior with some kind of setting or class override or something? I think sometimes we want to assert that we really get precise decimals when we're running on a single shard just to make sure kahan is plugged in. At least, I think that's true.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 27, 2024

Could we opt into this behavior with some kind of setting or class override or something? I think sometimes we want to assert that we really get precise decimals when we're running on a single shard just to make sure kahan is plugged in.

Great suggestion. I will update the PR.

@dnhatn dnhatn requested a review from nik9000 June 28, 2024 06:38
@dnhatn
Copy link
Member Author

dnhatn commented Jun 28, 2024

@nik9000 Can you take another look? Thank you!

Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nhat!

This should close #108560 and the 4 related issues, neat!

@not-napoleon
Copy link
Member

We discussed this a while ago, and decided we didn't want to do this. See https://github.com/elastic/elasticsearch-internal/pull/1333#issuecomment-1665615734 What's changed?

@alex-spies
Copy link
Contributor

Not sure if it was clear back then that our test would run against so many different environments. Esp. in multi-node scenarios (Serverless), there's essentially always going to be a small amount of non-determinism. IMHO rounding to 10 decimal places is reasonable to deal with this, esp. since per default (single node tests) rounding is disabled.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 28, 2024

@not-napoleon Yes, the difference is in Nik's suggestion to enable rounding conditionally. We disable rounding in single-node tests but enable it in multi-node tests. This allows us to still verify the precision of double values in single-node tests. Moreover, the tolerance used in multi-node tests in the PR is quite small, around 1E-10.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 28, 2024

Thanks everyone!

@dnhatn dnhatn changed the title Tolerate floating point precision in CSV Tests Tolerate floating point precision in multi node tests Jun 28, 2024
@dnhatn dnhatn merged commit 3cb77c9 into elastic:main Jun 28, 2024
@dnhatn dnhatn deleted the close-to branch June 28, 2024 15:41
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.14

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jun 28, 2024
Some assertions with floating-point values are failing on serverless 
because we run tests with three shards with serverless. This can cause
variations in precision because data may arrive in different orders. For
example, sum([a, b, c]) can yield a different precision than sum([a, c,
b]). This change introduces tolerance for precision differences beyond
e-10, which should be acceptable for ESQL.
elasticsearchmachine pushed a commit that referenced this pull request Jun 28, 2024
)

Some assertions with floating-point values are failing on serverless 
because we run tests with three shards with serverless. This can cause
variations in precision because data may arrive in different orders. For
example, sum([a, b, c]) can yield a different precision than sum([a, c,
b]). This change introduces tolerance for precision differences beyond
e-10, which should be acceptable for ESQL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.14.3 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants