-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Fix error when requesting _type from fields option. #68413
Conversation
For 8.0 indices, the 'fields' option would throw an error when '_type' is requested. Since '_type' was removed in 8.0, it should instead behave as if the field didn't exist, where we don't error but return no values.
Pinging @elastic/es-search (Team:Search) |
* A field type representing the document type. This will be null for 8.0 indices, | ||
* which don't have a type and do not support using the _type field in searches. | ||
*/ | ||
@Nullable private final TypeFieldType typeFieldType; |
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.
@romseygeek I was wondering why we still have this logic on 'master' to handle searches on _type
? Should it instead be removed completely? My approach here is to allow using _type
against 7.x indices, but for 8.x indices act as though the field doesn't exist. I'm not sure this makes sense -- maybe we should just remove this compatibility logic.
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.
+1 to removing it entirely in master. I think that was actually my original intention and I just never got round to it after I made the latest round of changes to TypeFieldType.
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.
Great, then I'll close this PR and open a new one to remove TypeFieldType
entirely.
For context, what is the plan for handling 7.x API compatibility, where I assume _type
will be allowed in searches? (I think I've asked you this 3+ times, I just always seem to forget...)
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 think it has changed a few times, @pgomulka will have a better idea of what the current plan is.
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.
@jtibshirani We plan to support _type
fields when a request was made with compatible API. if it was on a request, or part of a path it would be parsed but the value ignored. If there will be a need to return a _type
field on a response, we will return _doc
.
It is great you are asking, I am planning to create few example Compatible API for types removal and ask for feedback (I have few stale PRs with examples, can share offline if you like)
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.
Thanks @pgomulka -- I will ping you offline to understand the plan. My question is if I can remove support for _type
in searches entirely from master, or if I need to leave it in to coordinate with your plans.
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.
We discussed offline and agreed that the compatibility layer should continue to support _type
within searches. However @pgomulka was fine with us removing this for now, and then later figuring out the best strategy for retaining compatibility (which may involve restoring some pieces).
@elasticmachine update branch |
Closing in favor of #68564. |
For 8.0 indices, the 'fields' option would throw an error when '_type' is
requested. Since '_type' was removed in 8.0, it should instead behave as if the
field didn't exist, where we don't error but return no values.
Closes #68311.