Search: Simply SingleFieldsVisitor #34052
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`SingleFieldsVisitor` is meant to load a single stored field but it manages to be quite complex to reason about because it inherits from our "basic" `FieldsVisitor` which is designed to load many fields. This breaks that inheritance and adds logic to `SingleFieldsVisitor` so it can be properly stand alone. While this amounts to more lines of code they ought to be significantly easier to reason about.
nik9000
added
review
:Search/Search
|
Pinging @elastic/es-search-aggs |
nik9000
commented
Sep 25, 2018
| } catch (IOException e) { | ||
| throw new ElasticsearchParseException("failed to load field [{}]", e, name); | ||
| List<Object> values; | ||
| if (TypeFieldMapper.NAME.equals(data.fieldType().name())) { |
There was a problem hiding this comment.
This approach will not work in 6.x which has to be compatible with indices created in 5.x but I think it is quite sensible in master.
nik9000
commented
Sep 25, 2018
| getResult.docIdAndVersion().reader.document(getResult.docIdAndVersion().docId, visitor); | ||
| assertEquals(Arrays.asList(testFieldValue), visitor.fields().get("test")); |
There was a problem hiding this comment.
I suspect I could just call document(getResult.docIdAndVersion()) here and save some ceremony.
jpountz
approved these changes
Sep 26, 2018
|
Now that I'm looking at the backport more closely, I'm tempted to not pull this back to 6.x at all. |
kcm
pushed a commit
that referenced
this pull request
Oct 30, 2018
`SingleFieldsVisitor` is meant to load a single stored field but it manages to be quite complex to reason about because it inherits from our "basic" `FieldsVisitor` which is designed to load many fields. This breaks that inheritance and adds logic to `SingleFieldsVisitor` so it can be properly stand alone. While this amounts to more lines of code they ought to be significantly easier to reason about.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SingleFieldsVisitoris meant to load a single stored field but itmanages to be quite complex to reason about because it inherits from our
"basic"
FieldsVisitorwhich is designed to load many fields. Thisbreaks that inheritance and adds logic to
SingleFieldsVisitorso it canbe properly stand alone. While this amounts to more lines of code they
ought to be significantly easier to reason about.