-
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
Enable collapse on unsigned_long field #63495
Enable collapse on unsigned_long field #63495
Conversation
Collapse was not working on unsigned_long field, as collapsing was enabled only on KeywordFieldType and NumberFieldType. This introduces a new method `collapseType` to MappedFieldType, that is checked to decide if collapsing should be enabled. Relates to elastic#60050
Pinging @elastic/es-search (:Search/Mapping) |
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
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.
This looks good, I only left one comment regarding error messages.
throw new IllegalArgumentException("unknown type for collapse field `" + field + | ||
"`, only keywords and numbers are accepted"); | ||
if (fieldType.collapseType() == CollapseType.NONE) { | ||
throw new IllegalArgumentException("collapse is not supported on this field type"); |
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 we keep adding the field name to the error message to make it more actionable? Maybe we also have the actual field type available here to add to the error?
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.
@cbuescher Thanks for the feedback, addressed in 621992c
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 for the changes, LGTM
Collapse was not working on unsigned_long field, as collapsing was enabled only on KeywordFieldType and NumberFieldType. This introduces a new method `collapseType` to MappedFieldType, that is checked to decide if collapsing should be enabled. Relates to elastic#60050
Collapse was not working on unsigned_long field, as collapsing was enabled only on KeywordFieldType and NumberFieldType. This introduces a new method `collapseType` to MappedFieldType, that is checked to decide if collapsing should be enabled. Relates to #60050
Collapse was not working on unsigned_long field,
as collapsing was enabled only on KeywordFieldType and NumberFieldType.
This introduces a new method
collapseType
to MappedFieldType,that is checked to decide if collapsing should be enabled.
Relates to #60050