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

[Maps] filtered out docs with empty entity ids for tracks and top-hits layers #107680

Merged
merged 4 commits into from
Aug 10, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 4, 2021

Fixes #105599

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 4, 2021
@nreese nreese requested a review from thomasneirynck August 4, 2021 18:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@nreese
Copy link
Contributor Author

nreese commented Aug 4, 2021

Failed functional test documents source - geo top hits - split on scripted field - should display top hits per entity highlights an interesting property of the filter, It removed the 0 bucket. Not sure why. The bucket below is missing when the PR is applied.

{
            "key": "0",
            "doc_count": 22,
            "entityHits": {
              "hits": {
                "total": {
                  "value": 22,
                  "relation": "eq"
                },
                "max_score": null,
                "hits": [
                  {
                    "_index": "logstash-2015.09.22",
                    "_id": "AU_x3_g3GFA8no6QjkA4",
                    "_score": null,
                    "fields": {
                      "@timestamp": [
                        "2015-09-22T00:51:30.049Z"
                      ],
                      "utc_time": [
                        "2015-09-22T00:51:30.049Z"
                      ],
                      "relatedContent.article:modified_time": [
                        "2014-11-26T04:09:29.000Z"
                      ],
                      "geo.coordinates": [
                        "37.8583055594936, -80.3994722943753"
                      ],
                      "relatedContent.article:published_time": [
                        "2006-02-19T03:02:20.000Z"
                      ]
                    },
                    "sort": [
                      1442883090049
                    ]
                  }
                ]
              }
            }
          },

Generated filter.

"must_not": [
        {
          "script": {
            "script": {
              "source": "boolean compare(Supplier s, def v) {return s.get() == v;}compare(() -> { doc['@timestamp'].value.getHour() }, params.value);",
              "lang": "painless",
              "params": {
                "value": 0
              }
            }
          }
        }
      ]

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Aug 9, 2021

this is because of

For scripted-fields, it looks at the field-type and coerces the value to a number.

Wondering if we should use the exist-filter here:

export const buildExistsFilter = (field: IndexPatternFieldBase, indexPattern: IndexPatternBase) => {
. (although, this would only help with sparsity, but not with empty-values)

Also - to. be fair, I'm not sure if this is a bug perse, since this hits an edge-case (ie. users using a numeric pivot-field, which I would guess is very rare). I think this PR could still capture 95% of use-cases, if we add this filter only for fields of type string.

@nreese
Copy link
Contributor Author

nreese commented Aug 10, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Aug 10, 2021

I think this PR could still capture 95% of use-cases, if we add this filter only for fields of type string.

I have added a check to ensure the field is a string before adding the filters

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx, this is a nice one

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.2MB 3.2MB +544.0B
Unknown metric groups

References to deprecated APIs

id before after diff
maps 604 619 +15

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nreese nreese added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 10, 2021
@nreese nreese merged commit f596f89 into elastic:master Aug 10, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 10, 2021
…s layers (elastic#107680)

* [Maps] filtered out docs with empty entity ids for tracks and top-hits layers

* eslint

* add type check for string fields

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 10, 2021
…s layers (#107680) (#108100)

* [Maps] filtered out docs with empty entity ids for tracks and top-hits layers

* eslint

* add type check for string fields

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Nathan Reese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Docs with empty entity ids for tracks or top-hits layers should be filtered out
4 participants