-
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
Use mappings to format doc-value fields by default. #30831
Conversation
Doc-value fields now return a value that is based on the mappings rather than the script implementation by default. This deprecates the special `use_field_mapping` docvalue format which was added in elastic#29639 only to ease the transition to 7.x and it is not necessary anymore in 7.0.
Pinging @elastic/es-search-aggs |
@colings86 I assigned you for the review since it is a follow-up to #29639 which you already reviewed. |
This will be necessary for the `docvalue_fields` option to work correctly once we use the field's doc-value format to format doc-value fields. Binary values are formatted as base64-encoded strings. This issue was found in the PR testing of elastic#30831.
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 left a couple of minor comments but otherwise LGTM
obtained in 6.x with the special `use_field_mapping` format. This is mostly a | ||
change for date fields, which are now formatted based on the format that is | ||
configured in the mappings by default. This behavior can be changed by | ||
specifying a <<search-request-docvalue-fields,`format`>> with the doc-value |
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.
with
-> within
?
version: " - 6.3.99" | ||
reason: "object notation for docvalue_fields was introduced in 6.4" | ||
version: " - 6.99.99" | ||
reason: "Triggers warnings before 7.0" |
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.
Not sure but should we have a test to check for warnings that runs on 6.4-7.0 versions for the mixed cluster bwc tests?
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.
A test that would have "6.3.99 - 6.99.99" as a version would never run since 7.0 is not included?
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.
hmmm I think I might have a slightly misunderstanding on how the mixed cluster tests work then as I thought we set up a mixed cluster (let say 5 modes with 3 as 6.x and 2 as 7.0) and then we ran the YAML tests against the 6.x nodes using the 6.x version to work out which tests to run. Though now that I'm writing this I realise that as long as the 6.x version tests that the warning is sent (which it does) it should be enough and we probably shouldn't have 7.0 effectively test somethign in 6.x.
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.
Weeps I had missed that you were talking specifically about the bw tests. Yes, my preference would be to not test the warning, especially as things might depend on which node acts as a coordinating node for the search request.
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 once ci passes then
@elasticmachine run gradle build tests |
* master: (29 commits) Fix limit on retaining sequence number (elastic#37992) Docs test fix, wait for shards active. Revert "Revert "Documented default values for index follow request parameters. (elastic#37917)"" Revert "Documented default values for index follow request parameters. (elastic#37917)" Ensure date parsing BWC compatibility (elastic#37929) SQL: Skip the nested and object field types in case of an ODBC request (elastic#37948) Use mappings to format doc-value fields by default. (elastic#30831) Give precedence to index creation when mixing typed templates with typeless index creation and vice-versa. (elastic#37871) Add classifier to tar.gz in docker compose (elastic#38011) Documented default values for index follow request parameters. (elastic#37917) Fix fetch source option in expand search phase (elastic#37908) Restore a noop _all metadata field for 6x indices (elastic#37808) Added ccr to xpack usage infrastructure (elastic#37256) Fix exit code for Security CLI tools (elastic#37956) Streamline S3 Repository- and Client-Settings (elastic#37393) Add version 6.6.1 (elastic#37975) Ensure task metadata not null in follow test (elastic#37993) Docs fix - missing callout Types removal - deprecate include_type_name with index templates (elastic#37484) Handle completion suggestion without contexts ...
This function is unused now that we format the docvalue fields with the default formatter on the field (elastic#30831)
This function is unused now that we format the docvalue fields with the default formatter on the field (#30831)
* Remove use_field_mapping from integration test Relates: elastic/elasticsearch#30831 * Update expected serialized JSON
Doc-value fields now return a value that is based on the mappings rather than
the script implementation by default.
This deprecates the special
use_field_mapping
docvalue format which was addedin #29639 only to ease the transition to 7.x and it is not necessary anymore in
7.0.
Closes #26948