-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add block loader from stored field and source for ip field #126644
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
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
Hi @lkts, I've created a changelog YAML for you. |
| } | ||
| return null; | ||
|
|
||
| if (isStored()) { |
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 added this because it was very easy to do. I am not sure if there are any cases when this is not beneficial? Some existing implementations don't bother and just go straight to loading from source.
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.
👍 - seems easy to just do in this pr.
|
|
||
| @Override | ||
| public BlockLoader blockLoader(BlockLoaderContext blContext) { | ||
| if (hasDocValues()) { |
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.
Should i check for FieldExtractPreference.DOC_VALUES here?
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.
No, I don't think we have to. This only seems to be used in geo based field types.
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.
You are right, sorry, i actually meant FieldExtractPreference.STORED.
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.
it would be nice to check, yes.
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.
For context - that's the user asking you to load from _source. If you ignore it you are ignoring their kind request. You are sure allowed, but you probably shouldn't ignore it.
martijnvg
left a comment
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 👍
|
|
||
| @Override | ||
| public BlockLoader blockLoader(BlockLoaderContext blContext) { | ||
| if (hasDocValues()) { |
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.
No, I don't think we have to. This only seems to be used in geo based field types.
| } | ||
| return null; | ||
|
|
||
| if (isStored()) { |
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.
👍 - seems easy to just do in this pr.
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
|
@lkts is this one still backport_pending? |
|
This is merged to 8.19. |
This PR adds usage of
FallbackSyntheticSourceBlockLoadertoipfield. It also adds logic to load from a stored field and stored source since it was missing.