-
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
Search: Support of wildcard on docvalue_fields #32980
Conversation
For consistency with stored_fields, docvalue_fields should support the use of wildcards. See also: elastic#26390 Closes elastic#26299
Pinging @elastic/es-search-aggs |
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 @iverase , it looks good overall.
I left a comment regarding the error message when too many docvalue_fields are requested. Can you also update the documentation to explain this new behavior in:
docs/reference/search/request/docvalue-fields.asciidoc
?
+ "] but was [" + source.docValueFields().size() + "]. This limit can be set by changing the [" | ||
+ IndexSettings.MAX_DOCVALUE_FIELDS_SEARCH_SETTING.getKey() + "] index level setting."); | ||
"Trying to retrieve too many docvalue_fields. Must be less than or equal to: [" + maxAllowedDocvalueFields | ||
+ "] but was [" + source.docValueFields().size() + "]. This limit can be set by changing the [" |
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 it be docValueFields().size()
since source.docValueFields().size()
contains the patterns ?
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 totally right :)
behavior of doc values fields.
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, I only left a comment about the docs snippet.
"docvalue_fields" : [ | ||
{ | ||
"field": "*field", <1> | ||
"format": "use_field_mapping" <2> |
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 option is deprecated in 7.x so I suspect this will make the docs tests fail. We could do just docvalue_fields: [ "*field" ]
.
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.
Shall I update the docs or this should be addressed in a different issue?
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.
Sorry this is my bad, I assumed that #30831 had been merged, which is not true. There is nothing to fix in your PR.
* es/master: (62 commits) [DOCS] Add docs for Application Privileges (#32635) Add versions 5.6.12 and 6.4.1 Do NOT allow termvectors on nested fields (#32728) [Rollup] Return empty response when aggs are missing (#32796) [TEST] Add some ACL yaml tests for Rollup (#33035) Move non duplicated actions back into xpack core (#32952) Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086 Use `addIfAbsent` instead of checking if an element is contained TESTS: Fix Random Fail in MockTcpTransportTests (#33061) HLRC: Fix Compile Error From Missing Throws (#33083) [DOCS] Remove reload password from docs cf. #32889 HLRC: Add ML Get Buckets API (#33056) Watcher: Improve error messages for CronEvalTool (#32800) Search: Support of wildcard on docvalue_fields (#32980) Change query field expansion (#33020) INGEST: Cleanup Redundant Put Method (#33034) SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910) Fix the default pom file name (#33063) Switch ml basic tests to new style Requests (#32483) Switch some watcher tests to new style Requests (#33044) ...
* es/6.x: (58 commits) [DOCS] Add docs for Application Privileges (#32635) Add versions 5.6.12 and 6.4.1 [Rollup] Return empty response when aggs are missing (#32796) [TEST] Add some ACL yaml tests for Rollup (#33035) Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086 Use `addIfAbsent` instead of checking if an element is contained HLRC: Fix Compile Error From Missing Throws (#33083) [DOCS] Remove reload password from docs cf. #32889 Use a dedicated ConnectionManger for RemoteClusterConnection (#32988) Watcher: Improve error messages for CronEvalTool (#32800) HLRC: Add ML Get Buckets API (#33056) Change query field expansion (#33020) Search: Support of wildcard on docvalue_fields (#32980) Add beta label to MSI on install Elasticsearch page (#28126) SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910) [DOCS] Drafts Elasticsearch 6.4.0 release notes (#33039) Fix the default pom file name (#33063) Fix backport of switch ml basic tests to new style Requests (#32483) Switch ml basic tests to new style Requests (#32483) Switch some watcher tests to new style Requests (#33044) ...
For consistency with stored_fields, docvalue_fields should support the use of wildcards.
See also: #26390
Closes #26299