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

Prevent long value saturation within scaled_float mappings #105790

Closed

Conversation

benwtrent
Copy link
Member

It is easy to have long value saturation and overflow with scaled_float and allowing users to saturate longs seems dangerous and confusing.

I would generally expect any saturation to be an input error by the user as it will then break subsequent queries against the field (see example range query in the issue).

This commit allows saturation to occur if the index was created in a previous version or if ignore_malformed: true.

closes: #105361

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.14.0 labels Feb 23, 2024
@brianseeders brianseeders added the :Search/Mapping Index mappings, including merging and defining field types label Mar 1, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label labels Mar 1, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I was wondering if we consider this a bugfix or an enhancement (the corresponding label is missing), as that would affect the bwc strategy.

@benwtrent
Copy link
Member Author

@javanna I do not know. But using range queries (one of the main reasons to have scaled_float) over saturated values just doesn't work.

We can update the docs separately to ensure that folks know this.

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.

I like the idea of failing but I don't know if we're ok with the semi-breaking nature. Opting folks into rejecting documents just by upgrading seems like something dangerous at this point.

@benwtrent
Copy link
Member Author

Opting folks into rejecting documents just by upgrading seems like something dangerous at this point.

I understand these concerns.

We can just document, "Hey, don't do this".

I definitely don't want to add a new parameter, and ignore_malformed seemed to fit this situation.

What do y'all think of adding a deprecation here? Seems like we should warn users and they shouldn't be doing this and if they want to keep doing it, they gotta allow ignore_malformed. @nik9000 @javanna

@benwtrent
Copy link
Member Author

For now, I am going to close this PR and create another one that is a docs update. We should be very clear, that saturation can happen and can return weirdness. Simply saying "hey, these fit in a long value" isn't enough for typical users.

@benwtrent benwtrent closed this Apr 26, 2024
@javanna javanna removed the v8.15.0 label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scale_float does not honor limits for long values
5 participants