-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 support for storing JSON fields. #34942
Add support for storing JSON fields. #34942
Conversation
263e4cc
to
90732e9
Compare
7794870
to
59bd0ca
Compare
Pinging @elastic/es-search-aggs |
if (fieldType().indexOptions() != IndexOptions.NONE) { | ||
XContentParser indexedFieldsParser = context.parser(); | ||
|
||
// If store is enabled, we've already consumed the content to produce the stored field. Here we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative here would be to add logic to JsonFieldParser
to generate the stored field as it's parsed. Although it avoids parsing the input twice as we do here, that approach turned turned out fairly messy and didn't separate out concerns as nicely. It also seems uncommon to enable stored fields, so I wasn't as concerned about the fact we will parse the input twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I don't think its worth the complexity of trying to produce the stored field content in the same pass as the index field for the rare case that the field is stored.
bb0884d
to
a6d8a49
Compare
59bd0ca
to
5221f18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
When 'store' is enabled, a single stored field is added containing the entire JSON object in pretty-printed format.