Skip to content

ESQL: Fix precision of scaled_float field values retrieved from stored source #122586

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.

Sign up for GitHub

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

Jump to bottom
Merged
lkts merged 9 commits into from
Feb 20, 2025
Merged

ESQL: Fix precision of scaled_float field values retrieved from stored source #122586

lkts merged 9 commits into from
Feb 20, 2025

Conversation

@kanoshiou
Copy link
Contributor

@kanoshiou kanoshiou commented Feb 14, 2025

This PR resolves the issue where different results were returned for fields using scaled_float depending on whether doc_values was enabled or disabled. This discrepancy occurred due to the extra imprecision introduced by the use of double in the calculations.

Closes #122547

@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team needs:triage Requires assignment of a team area label v9.1.0 labels Feb 14, 2025
@kanoshiou kanoshiou changed the title ESQL: Consistency in using scaled_float field ESQL: Fix inconsistency results in using scaled_float field Feb 14, 2025
@kanoshiou kanoshiou changed the title ESQL: Fix inconsistency results in using scaled_float field ESQL: Fix inconsistent results in using scaled_float field Feb 14, 2025
@lkts lkts added the :Analytics/ES|QL AKA ESQL label Feb 14, 2025
@elasticsearchmachine elasticsearchmachine added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed needs:triage Requires assignment of a team area label labels Feb 14, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 14, 2025

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

@kanoshiou kanoshiou marked this pull request as draft February 14, 2025 16:50
@lkts lkts mentioned this pull request Feb 14, 2025
Merged
@kanoshiou
Copy link
Contributor Author

kanoshiou commented Feb 15, 2025

I'm not sure if any additional tests are necessary, because I found a test that already covers the accuracy loss of scaled floats.

Please take a look if you have some time, @lkts. Thank you very much.

elasticsearch/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/extras/ScaledFloatFieldTypeTests.java

Lines 85 to 88 in a59c182

public void testRangeQuery() throws IOException {
// make sure the accuracy loss of scaled floats only occurs at index time
// this test checks that searching scaled floats yields the same results as
// searching doubles that are rounded to the closest half float

@kanoshiou kanoshiou marked this pull request as ready for review February 15, 2025 06:29
@alex-spies alex-spies mentioned this pull request Feb 17, 2025
Closed
@lkts lkts self-assigned this Feb 18, 2025
@lkts lkts added :StorageEngine/Mapping The storage related side of mappings auto-backport Automatically create backport pull requests when merged v8.19.0 labels Feb 18, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 18, 2025

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@lkts lkts added the >bug label Feb 18, 2025
@lkts lkts changed the title ESQL: Fix inconsistent results in using scaled_float field ESQL: Fix precision of scaled_float field values retrieved from stored source Feb 18, 2025
@lkts
Copy link
Contributor

lkts commented Feb 18, 2025 •
edited
Loading

Thank you @kanoshiou! Everything looks good to me. Can you please update ScaledFloatFieldBlockLoaderTests as well (it's new)? I have also started a CI run, let's wait for results there.

@lkts
Copy link
Contributor

lkts commented Feb 18, 2025

buildkite test this please

lkts
lkts reviewed Feb 18, 2025
View reviewed changes
x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec
emp_no:integer |height:double |height.float:double |height.half_float:double |height.scaled_float:double
10001 |2.03 |2.0299999713897705 |2.029296875 |2.0300000000000002
10090 |2.03 |2.0299999713897705 |2.029296875 |2.0300000000000002
10001 |2.03 |2.0299999713897705 |2.029296875 |2.03
Copy link
Contributor

@lkts lkts Feb 18, 2025

Choose a reason for hiding this comment

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

You need to add a new capability for this fix and use it in this test similar to how fix_parsing_large_negative_numbers is used above. This is needed to fix backward compatibility tests.

@kanoshiou
Copy link
Contributor Author

kanoshiou commented Feb 19, 2025

Thank you for your review, @lkts ! All comments have been addressed.

@lkts
Copy link
Contributor

lkts commented Feb 19, 2025

buildkite test this please

@lkts
Copy link
Contributor

lkts commented Feb 20, 2025

buildkite test this please

@lkts
Copy link
Contributor

lkts commented Feb 20, 2025

@kanoshiou there is one more test failure that i will take care of and we should be good to go.

@lkts lkts added v9.0.0 and removed v9.0.0 labels Feb 20, 2025
@lkts
Copy link
Contributor

lkts commented Feb 20, 2025

buildkite test this please

@lkts
Copy link
Contributor

lkts commented Feb 20, 2025

buildkite test this please

@lkts lkts merged commit de41d57 into elastic:main Feb 20, 2025
19 checks passed
@lkts lkts mentioned this pull request Feb 20, 2025
Merged
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 20, 2025

💚 Backport successful

Status Branch Result
✅ 8.x

elasticsearchmachine pushed a commit that referenced this pull request Feb 21, 2025
@lkts @kanoshiou
[8.x] ESQL: Fix precision of scaled_float field values retrieved fr…
d6e1ad7 
…om stored source (#122586) (#123080)

* ESQL: Fix precision of `scaled_float` field values retrieved from stored source (#122586)

* style

---------

Co-authored-by: kanoshiou <[email protected]>
@ChrisHegarty ChrisHegarty mentioned this pull request Feb 21, 2025
Merged
@alex-spies alex-spies mentioned this pull request Feb 21, 2025
Closed
@astefan
Copy link
Contributor

astefan commented Feb 21, 2025 •
edited
Loading

@lkts any reason why this hasn't been backported to 9.0 branch as well?

@lkts
Copy link
Contributor

lkts commented Feb 21, 2025

@astefan No particular reason. I figured it's not necessary given that there is no upgrade path that will "revert" this fix. E.g. you either go 9.0 -> 9.1 or 8.19 -> 9.1. I am happy to backport if needed.

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

Reviewers

@lkts lkts lkts approved these changes

Assignees

@lkts lkts

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >bug external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/Mapping The storage related side of mappings Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:StorageEngine v8.19.0 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slight inconsistency in ESQL using scaled_float field

4 participants

@kanoshiou @elasticsearchmachine @lkts @astefan