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.

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

Fix casting of scaled_float in sorts #57207

Merged
merged 3 commits into from
May 29, 2020
Merged

Fix casting of scaled_float in sorts #57207

merged 3 commits into from
May 29, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented May 27, 2020

Previously we'd get a ClassCastException when you tried to use
numeric_type on scaled_float. Oops! This cleans up the CCE and moves
some code around so the casting actually works.

@nik9000
Fix casting of scaled_float in sorts
572f090 
Previously we'd get a `ClassCastException` when you tried to use
`numeric_type` on `scaled_float`. Oops! This cleans up the CCE and moves
some code around so the casting actually works.
@nik9000 nik9000 added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.9.0 labels May 27, 2020
@nik9000 nik9000 requested a review from javanna May 27, 2020 12:47
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label May 27, 2020
@nik9000
Copy link
Member Author

nik9000 commented May 27, 2020

It'd be super reasonable for you to ask "why in the world would you want to cast a scale_float to long or a date or date_millis or whatever?" Its a great question! I believe this feature was added so you could sort results from two indices with different mappings. Folk often make indices for some time period and when they change the mapping of a field they don't re-index their old data because it'd take forever. This lets you merge those fields together for sorting. It comes up. Truly. Maybe doing it with a scaled_float doesn't come up often though!

@nik9000
Copy link
Member Author

nik9000 commented May 27, 2020

@elasticmachine, run elasticsearch-ci/2

@nik9000 nik9000 mentioned this pull request May 28, 2020
Closed
16 tasks
@javanna
Copy link
Member

javanna commented May 29, 2020

this makes sense to me but I think we should find a reviewer who's more familiar with mappings code than me :) maybe @romseygeek ?

@nik9000 nik9000 requested a review from romseygeek May 29, 2020 13:52
@nik9000
Copy link
Member Author

nik9000 commented May 29, 2020

So pinged!

@romseygeek
Copy link
Contributor

This isn't a bit of mappings that I know a lot about, alas - somebody who knows more about aggs than me is probably a better bet...

polyfractal
polyfractal approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@polyfractal polyfractal left a comment

Choose a reason for hiding this comment

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

Left a comment but otherwise LGTM. I'm far from an expert in this area of the code either, so grain of salt :)

server/src/main/java/org/elasticsearch/index/fielddata/plain/SortedNumericIndexFieldData.java
return comparatorSource(targetNumericType, missingValue, sortMode, nested)
.newBucketedSort(bigArrays, sortOrder, format, bucketSize, extra);
protected boolean sortRequiresCustomComparator() {
return numericType == NumericType.HALF_FLOAT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave a comment in here about why half_floats need a custom comparator? I'm assuming their encoding makes them non-ordered or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that is so. I'll dig and leave a comment.

jimczi
jimczi approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, nice cleanup

modules/mapper-extras/src/test/resources/rest-api-spec/test/scaled_float/10_basic.yml
@@ -97,8 +97,28 @@ setup:

- do:
search:
rest_total_hits_as_int: true
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks

@nik9000 nik9000 merged commit 7382f44 into elastic:master May 29, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request May 29, 2020
@nik9000
Fix casting of scaled_float in sorts (elastic#57207)
04714e1 
Previously we'd get a `ClassCastException` when you tried to use
`numeric_type` on `scaled_float`. Oops! This cleans up the CCE and moves
some code around so the casting actually works.
nik9000 added a commit that referenced this pull request May 29, 2020
@nik9000
Fix casting of scaled_float in sorts (#57207) (#57385)
f52e779 
Previously we'd get a `ClassCastException` when you tried to use
`numeric_type` on `scaled_float`. Oops! This cleans up the CCE and moves
some code around so the casting actually works.
@gashutos gashutos mentioned this pull request Feb 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@polyfractal polyfractal polyfractal approved these changes

@jimczi jimczi jimczi approved these changes

@javanna javanna Awaiting requested review from javanna

@romseygeek romseygeek Awaiting requested review from romseygeek

Assignees
No one assigned
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.9.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nik9000 @elasticmachine @javanna @romseygeek @polyfractal @jimczi @jakelandis