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

Add source fallback support for unsigned long mapped type #89349

Merged
merged 7 commits into from
Aug 17, 2022

Conversation

jdconrad
Copy link
Contributor

This change adds source fallback support for unsigned long by adding a new SourceValueFetcherSortedUnsignedLongIndexFieldData similar to the other numeric types.

@jdconrad jdconrad added >enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v8.5.0 labels Aug 15, 2022
@jdconrad jdconrad requested a review from romseygeek August 15, 2022 17:44
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Aug 15, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @jdconrad, I've created a changelog YAML for you.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

Thanks @jdconrad! I left a couple of comments.


import static org.elasticsearch.search.DocValueFormat.MASK_2_63;

public class SourceValueFetcherSortedUnsignedLongIndexFieldData extends SourceValueFetcherIndexFieldData<SortedNumericDocValues> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have some javadoc here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@Override
public boolean advanceExact(int doc) throws IOException {
sourceLookup.setSegmentAndDocument(leafReaderContext, doc);
values = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to create a new list every time here or can we reuse an existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to clear as suggested. Good catch.

@jdconrad
Copy link
Contributor Author

@romseygeek Thank you the first review. I've updated the PR based on your feedback.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM

@jdconrad jdconrad merged commit fe8e586 into elastic:main Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants