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

[KQL] Do not generate nested AST/Elasticsearch queries for same-level AND/OR clauses #93506

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Mar 3, 2021

Summary

Fixes #69649.
Fixes #89473.

This PR changes the KQL parser to no longer generate nested AST nodes for same-level and/or clauses. This also removes the recursive nature of the parsing, which eliminates the possibility of hitting the maximum call stack size limit.

For example, prior to this PR, a clause such as foo and bar and baz would generate the following AST (simplified for illustration purposes):

{
  "and": [
    {"is": [null, "foo"]},
    {
      "and": [
        {"is": [null, "bar"]},
        {"is": [null, "baz"]}
      ]
    }
  ]
}

Following this PR, it generates the following AST:

{
  "and": [
    {"is": [null, "foo"]},
    {"is": [null, "bar"]},
    {"is": [null, "baz"]}
  ]
}

The corresponding Elasticsearch queries generated from these ASTs have also been simplified, so that they don't use nested bool queries.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release note

KQL will no longer generate nested Elasticsearch bool queries for same-level AND/OR clauses.

@lukasolson lukasolson requested a review from a team as a code owner March 3, 2021 20:23
@lukasolson lukasolson self-assigned this Mar 3, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@lukasolson lukasolson requested a review from a team as a code owner March 3, 2021 22:46
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Changes to core tests LGTM

@lukasolson lukasolson requested review from a team as code owners March 4, 2021 21:20
@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Mar 4, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for walking me through the code and how to read the Peg file!

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM from uptime

lukasolson added a commit to lukasolson/kibana that referenced this pull request Mar 5, 2021
… AND/OR clauses (elastic#93506)

* [KQL] Do not generate nested AST for same-level AND/OR clauses

* Update tests

* Update snapshots
lukasolson added a commit to lukasolson/kibana that referenced this pull request Mar 5, 2021
… AND/OR clauses (elastic#93506)

* [KQL] Do not generate nested AST for same-level AND/OR clauses

* Update tests

* Update snapshots
@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / general / "before all" hook for "should contain the right query".Timeline query tab Query tab "before all" hook for "should contain the right query"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="title-c424c520-7e11-11eb-bca4-8dccd5f64726"]`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `Timeline query tab`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at Object.openTimelineById (http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/query_tab.spec.ts:16059:15)
    at Context.eval (http://localhost:6121/__cypress/tests?p=cypress/integration/timelines/query_tab.spec.ts:15045:28)

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 814.0KB 815.6KB +1.7KB

History

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

cc @lukasolson

lukasolson added a commit that referenced this pull request Mar 6, 2021
… AND/OR clauses (#93506) (#93862)

* [KQL] Do not generate nested AST for same-level AND/OR clauses

* Update tests

* Update snapshots
lukasolson added a commit that referenced this pull request Mar 6, 2021
… AND/OR clauses (#93506) (#93861)

* [KQL] Do not generate nested AST for same-level AND/OR clauses

* Update tests

* Update snapshots
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants