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.

Sign up for GitHub

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

Jump to bottom

[KQL] Fix node builder with empty array #155901

Merged
merged 2 commits into from
Apr 27, 2023
Merged

[KQL] Fix node builder with empty array #155901

merged 2 commits into from
Apr 27, 2023

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Apr 26, 2023 •
edited by kibanamachine
Loading

Summary

Fixes #95657.

Updates the logic for nodeBuilder when passing an empty array to nodeBuilder.and/nodeBuilder.or to actually return a KueryNode instead of undefined.

Checklist

@lukasolson lukasolson added Feature:KQL KQL release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.8.0 labels Apr 26, 2023
@lukasolson lukasolson self-assigned this Apr 26, 2023
@lukasolson lukasolson requested a review from a team as a code owner April 26, 2023 16:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@lukasolson lukasolson requested review from a team as code owners April 26, 2023 17:15
pmuellr
pmuellr approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

code LGTM, but will be leaving a question in the comments ...

@pmuellr
Copy link
Member

pmuellr commented Apr 26, 2023

I'm curious about this change. What happens when this is converted to ES?

Naively, if previously a clause was generating undefined, that property with the undefined value simply would not be passed to ES (eventually, when the JSON is created to send to ES). But now it will end up as a filter that - I think - will always be false?

Seems like this could break some things.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
kbnUiSharedDeps-srcJs 2.2MB 2.2MB +4.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 397 400 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 477 480 +3
total +5

History

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

cc @lukasolson

@lukasolson
Copy link
Member Author

I'm curious about this change. What happens when this is converted to ES?

The code for converting to ES DSL is here:

kibana/packages/kbn-es-query/src/kuery/ast/ast.ts

Lines 73 to 75 in 1cbf83f

if (!node || !node.type || !nodeTypes[node.type]) {
return toElasticsearchQuery(nodeTypes.function.buildNode('and', []), indexPattern);
}

We treat an undefined KQL node the same as we treat an and node with no conditions, which is converted to an empty bool filter clause (same as or node), so functionally this shouldn't change anything there.

kertal
kertal reviewed Apr 27, 2023
View reviewed changes
packages/kbn-es-query/src/kuery/node_types/node_builder.ts
return nodeTypes.function.buildNodeWithArgumentNodes('is', [
nodeTypes.literal.buildNode(fieldName),
typeof value === 'string' ? nodeTypes.literal.buildNode(value) : value,
]);
},
or: (nodes: KueryNode[]): KueryNode => {
return nodes.length > 1 ? nodeTypes.function.buildNode('or', nodes) : nodes[0];
return nodes.length === 1 ? nodes[0] : nodeTypes.function.buildNode('or', nodes);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this also work?

Suggested change
return nodes.length === 1 ? nodes[0] : nodeTypes.function.buildNode('or', nodes);
return nodeTypes.function.buildNode('or', nodes);

Copy link
Member Author

Choose a reason for hiding this comment

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

Spoke to you about this offline, but just adding a note here. Returning the first node is a slight optimization (don't need to build an extra node, and don't have to wrap the ES query in an additional unnecessary bool clause).

kertal
kertal approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@kertal kertal left a comment •
edited by lukasolson
Loading

Choose a reason for hiding this comment

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

LGTM, thx for taking care of this 🙇. didn't test, but unit tests do, just a tiny question I've been adding

@lukasolson lukasolson merged commit 654287b into elastic:main Apr 27, 2023
@lukasolson lukasolson added v8.9.0 and removed v8.8.0 labels Apr 27, 2023
@kibanamachine kibanamachine added backport:skip This commit does not require backporting labels Apr 27, 2023
@kibanamachine kibanamachine mentioned this pull request Apr 27, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 27, 2023
@lukasolson
[KQL] Fix node builder with empty array (elastic#155901)
dec4672 
## Summary

Fixes elastic#95657.

Updates the logic for `nodeBuilder` when passing an empty array to
`nodeBuilder.and`/`nodeBuilder.or` to actually return a `KueryNode`
instead of `undefined`.

### Checklist

- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit 654287b)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 27, 2023
@kibanamachine @lukasolson
[8.8] [KQL] Fix node builder with empty array (#155901) (#156077)
f657db7 
# Backport

This will backport the following commits from `main` to `8.8`:
- [[KQL] Fix node builder with empty array
(#155901)](#155901)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Lukas
Olson","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-27T18:28:12Z","message":"[KQL]
Fix node builder with empty array (#155901)\n\n## Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/95657.\r\n\r\nUpdates the logic
for `nodeBuilder` when passing an empty array
to\r\n`nodeBuilder.and`/`nodeBuilder.or` to actually return a
`KueryNode`\r\ninstead of `undefined`.\r\n\r\n### Checklist\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"654287b3adc928dd1582998504b7ed043259c3ae","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:KQL","release_note:skip","backport:skip","Team:DataDiscovery","v8.9.0"],"number":155901,"url":"https://github.com/elastic/kibana/pull/155901","mergeCommit":{"message":"[KQL]
Fix node builder with empty array (#155901)\n\n## Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/95657.\r\n\r\nUpdates the logic
for `nodeBuilder` when passing an empty array
to\r\n`nodeBuilder.and`/`nodeBuilder.or` to actually return a
`KueryNode`\r\ninstead of `undefined`.\r\n\r\n### Checklist\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"654287b3adc928dd1582998504b7ed043259c3ae"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155901","number":155901,"mergeCommit":{"message":"[KQL]
Fix node builder with empty array (#155901)\n\n## Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/95657.\r\n\r\nUpdates the logic
for `nodeBuilder` when passing an empty array
to\r\n`nodeBuilder.and`/`nodeBuilder.or` to actually return a
`KueryNode`\r\ninstead of `undefined`.\r\n\r\n### Checklist\r\n\r\n- [
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials\r\n- [x] [Unit
or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"654287b3adc928dd1582998504b7ed043259c3ae"}}]}]
BACKPORT-->

Co-authored-by: Lukas Olson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@pmuellr pmuellr pmuellr approved these changes

Assignees

@lukasolson lukasolson

Labels
backport:skip This commit does not require backporting Feature:KQL KQL release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.8.0 v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Kibana] NodeBuilder functions return undefined when input is an empty array.
6 participants
@lukasolson @elasticmachine @pmuellr @kibana-ci @kibanamachine @kertal