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

SearchSource: fix docvalue_fields and fields intersection logic #46724

Merged
merged 3 commits into from
Oct 4, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Sep 26, 2019

There is a bug in SearchSource with the existing docvalue_fields and fields intersection logic.

https://github.com/elastic/kibana/blob/7.4/src/legacy/ui/public/courier/search_source/search_source.js#L573

flatData.body.docvalue_fields = _.intersection(flatData.body.docvalue_fields, fields);

The logic assumes that both flatData.body.docvalue_fields and fields is an array of strings. That is not the case. flatData.body.docvalue_fields is an array of objects as specified here.

This PR updates the intersection logic to account for the fact that docvalue_fields is an array of objects.

The PR also updates how the default docvalue_fields is assigned. The old logic unioned the provided docvalue_fields and those from getComputedFields. This made it impossible to remove getComputedFields.docvalue_fields that are also include in fields. If a search source provides docvalue_fields then it does not make sense to union with getComputedFields.docvalue_fields.

Below is the use case for the maps application that spurred these changes. The maps application would like to receive the date as epoch_millis so it can be treated like a number to style markers based on age. The maps application would also like to receive the geo_point geometry from doc_values so we do not have to parse the value from _source since there are 5 different accepted formats.

const initialSearchSourceState = {
  docvalue_fields: [
    {
      field: "@timestamp",
      format: "epoch_millis"
    },
    {
      field: "geo.coordinates",
    }
  ],
};
const searchSource = new SearchSource(initialSearchSourceState);

With the updates our request and response are what are expected

// ES _search request body
{
  "docvalue_fields": [
    {
      "field": "@timestamp",
      "format": "epoch_millis"
    },
    {
      "field": "geo.coordinates"
    }
  ],
  "size": 10000,
}

// ES _search response hit example
"hits": [
  {
    "_index": "kibana_sample_data_logs",
    "_type": "_doc",
    "_id": "R4P3bW0BXBg7ubMI10Mb",
    "_score": 0,
    "_source": {},
    "fields": {
      "geo.coordinates": [
        "47.547698873095214, -116.18850083090365"
      ],
      "@timestamp": [
        "1569522434527"
      ]
    }
  }
]

When the provided docvalue_fields are unioned with getComputedFields the following is returned. Notice the wasted bandwidth because @timestamp is now returned to 2 formats

// ES _search request body
{
  "docvalue_fields": [
    {
      "field": "@timestamp",
      "format": "epoch_millis"
    },
    {
      "field": "geo.coordinates"
    },
    {
      "field": "@timestamp",
      "format": "date_time"
    }
  ],
  "size": 10000,
}

// ES _search response hit example
"hits": [
  {
    "_index": "kibana_sample_data_logs",
    "_type": "_doc",
    "_id": "R4P3bW0BXBg7ubMI10Mb",
    "_score": 0,
    "_source": {},
    "fields": {
      "geo.coordinates": [
        "47.547698873095214, -116.18850083090365"
      ],
      "@timestamp": [
        "1569522434527",
        "2019-09-26T18:27:14.527Z"
      ]
    }
  }
]

cc @nickpeihl

@nreese nreese added release_note:fix [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.5.0 labels Sep 26, 2019
@nreese nreese requested review from lukasolson and Bargs September 26, 2019 18:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese requested review from a team as code owners September 30, 2019 12:17
@nreese nreese requested a review from a team September 30, 2019 12:17
@nreese nreese requested review from a team as code owners September 30, 2019 12:17
@elasticcla
Copy link

Hi @nreese, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@nreese nreese removed request for a team September 30, 2019 12:20
@nreese nreese removed request for a team September 30, 2019 12:20
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese requested a review from Bargs October 4, 2019 14:35
@nreese
Copy link
Contributor Author

nreese commented Oct 4, 2019

@Bargs @lukasolson This is ready for review

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM

@nreese nreese merged commit 3e9b0c8 into elastic:master Oct 4, 2019
nreese added a commit to nreese/kibana that referenced this pull request Oct 4, 2019
…tic#46724)

* SearchSource: fix docvalue_fields and fields intersection logic

* update filter logic to handle docvalue_fields that are just strings
nreese added a commit that referenced this pull request Oct 4, 2019
…) (#47370)

* SearchSource: fix docvalue_fields and fields intersection logic

* update filter logic to handle docvalue_fields that are just strings
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 7, 2019
… into console-token-iterator

* 'console-token-iterator' of github.com:jloleysens/kibana: (184 commits)
  [functional/services] update webdriver lib and types (elastic#47381)
  Standardizing IconField implementation across the app (elastic#47196)
  Move ui/value_suggestions ⇒ NP data plugin (elastic#45762)
  Remove ui/persisted_log - Part 2 (elastic#47236)
  Update gulp related packages (elastic#47421)
  Update dependency idx to ^2.5.6 (elastic#47399)
  try running fewer jobs in parallel on the same worker (elastic#47403)
  Update webpack related packages (elastic#47402)
  Update jsonwebtoken related packages (elastic#47400)
  Update gulp related packages (major) (elastic#46665)
  Update dependency prettier to ^1.18.2 (elastic#47340)
  Update dependency @types/puppeteer to ^1.20.1 (elastic#47339)
  Update dependency @elastic/elasticsearch to ^7.4.0 (elastic#47338)
  Update dependency tar-fs to ^1.16.3 (elastic#47341)
  [Code] Code Integrator Component (elastic#47180)
  [Canvas][i18n] Sidebar (elastic#46090)
  Generate uuid in task Manager as Kibana uuid may not yet have been initialised
  [Code] Embedded Code Snippet Component (elastic#47183)
  Revert "Add pipeline for flaky test runner job (elastic#46740)"
  SearchSource: fix docvalue_fields and fields intersection logic (elastic#46724)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants