Skip to content

Conversation

@parkertimmins
Copy link
Contributor

@parkertimmins parkertimmins commented Aug 25, 2025

A recent change to the numeric matches uses specific Java types for the related matcher rather than Integer for all integer types and Double for all double types. The same needs to be done for null values for equality to work correctly.

Fixes #133560
Fixes #133499
Fixes #133498

Fixes #133497
Fixes #133518
Fixes #133517

Fixes #133516
Fixes #133505
Fixes #133504

Fixes #133503
Fixes #133520
Fixes #132602

Fixes #132601
Fixes #132600

@parkertimmins parkertimmins requested a review from lkts August 25, 2025 22:35
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v9.2.0 labels Aug 25, 2025
@parkertimmins
Copy link
Contributor Author

This could also be done by extending NumericMatcher with a separate LongMatcher, ShortMatcher, ByteMatcher, and FloatMatcher. I'm pretty indifferent to which way to do it.

@parkertimmins parkertimmins added >test Issues or PRs that are addressing/adding tests :StorageEngine/Mapping The storage related side of mappings labels Aug 25, 2025
@elasticsearchmachine elasticsearchmachine added Team:StorageEngine and removed needs:triage Requires assignment of a team area label labels Aug 25, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@lkts lkts left a comment

Choose a reason for hiding this comment

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

I do wonder if it is more robust and readable to have a specific matcher for every number type. I think overall i prefer that but i haven't looked deeply to evaluate that. NumberMatcher existed because the logic was the same but now it isn't.

}

yield s;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While testing, found that the unsigned long mapper can accept unicode numbers and needs to be updates.

@parkertimmins
Copy link
Contributor Author

@lkts That makes sense. Separate classes are perhaps easier to understand. What do you think of this?

@lkts
Copy link
Contributor

lkts commented Aug 26, 2025

This looks great, thanks for incorporating the suggestion.

@parkertimmins parkertimmins merged commit e543a0b into elastic:main Aug 27, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment