Add an error margin when comparing floats. #129721
Conversation
gmarouli
added
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| } | ||
|
|
||
| for (int i = 0; i < normalizedActual.size(); i++) { | ||
| if (floatsEquals(normalizedActual.get(i), normalizedExpected.get(i))) { |
There was a problem hiding this comment.
So if floatsEquals(...) returns true then there is no match? Maybe rename floatsEquals(...) to floatsNoEquals(...)?
There was a problem hiding this comment.
oh boy, my bad. Actually I prefer to keep it floatsEquals and fix the code because its more intuitive.
...work/src/main/java/org/elasticsearch/datageneration/matchers/source/DynamicFieldMatcher.java
Outdated
Show resolved
Hide resolved
|
Thank you @martijnvg for the review, such an oversight the method name and implementation 🙃 . I run the test locally with What do you think? |
Yes, that should give good confidence. |
| if (normalizedActual.size() != normalizedExpected.size()) { | ||
| return noMatchSupplier.get(); | ||
| } | ||
|
|
||
| for (int i = 0; i < normalizedActual.size(); i++) { | ||
| if (floatEquals(normalizedActual.get(i), normalizedExpected.get(i)) == false) { | ||
| return noMatchSupplier.get(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Both of these checks are on the normalized lists, which have null values removed. This means if there's an extra null somewhere, or if the null is in a different position between the expected and actual, we wouldn't catch it here. Is this intentional?
We add a margin of error when comparing floats to the
DynamicFieldMatcherto account for a small loss of accuracy when loading fields from synthetic source.Furthermore, we switched the set to a list to take the order of array elements also into account since it's supported.
Fixes: #129527, #129528, #129529