Skip to content
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

Pass runtime_fields from FieldCapabilitiesRequest to FieldCapabilitiesIndexRequest #69853

Merged

Conversation

przemekwitek
Copy link
Contributor

@przemekwitek przemekwitek commented Mar 3, 2021

This PR makes the runtime_fields map passed down to remote FieldCapabilitiesRequests.
Additionally:

  • makes runtimeFields member variable final in FieldCapabilitiesIndexRequest
  • makes the style of reading field consistent in FieldCapabilitiesRequest

Relates to #68904

Marked as >non-issue as this is a fix for an unreleased feature.

@przemekwitek przemekwitek added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.12.0 v7.13.0 labels Mar 3, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Mar 3, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me, it would be great to have a way to test this in the "normal" case though (when the remote cluster is on 7.12 or above). I don't know if we have the appropriate test infra though, so I'm currently looking for more information. If this proves to be difficult we should probably merge this for now and open a follow up to add those tests later.

@przemekwitek
Copy link
Contributor Author

I'm not sure about the test suite for _field_caps but there is a precedence of testing transform in multi-cluster env, see e.g.: x-pack/qa/multi-cluster-tests-with-security/src/test/resources/rest-api-spec/test/multi_cluster/80_transform.yml

@cbuescher
Copy link
Member

@przemekwitek I found yaml tests for "field_caps" under ':qa:multi-cluster-search' and briefly played around with it (see cbuescher@93ca26c). Adding something basic like this with a runtime_mapping that changes a remote fields type should be enough to test the basic propagation I think.

@cbuescher cbuescher self-assigned this Mar 3, 2021
@przemekwitek
Copy link
Contributor Author

Adding something basic like this with a runtime_mapping that changes a remote fields type should be enough to test the basic propagation I think.

I verified that your test fails without the fix but passes with the fix.
I added the test to this PR.
Thank you for your help!

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks again for catching this. LGTM pending CI is green.

number:
type: keyword
script:
source: "emit(doc['number'].value)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering how this is not throwing an error for trying to access doc_values of the field that is being defined. Would it make sense to use a different field name? You can potentially even skip the script.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I see that this is also testing shadowing the existing mapped field. Maybe that is not required though as it's besides the scope of this test. We could define a new field as part of runtime_mappings and check that field_caps returns it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (skipped the script part).

I think the script is just not executed during _field_caps so it's cleaner not to provide it in the first place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right that's it and that is why it was not causing an error :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.12.0 v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants