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

adding kibana filter expression functions #94069

Merged
merged 9 commits into from
Mar 23, 2021

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Mar 9, 2021

Summary

kibanaFilter, rangeFilter, existsFilter and phraseFilter expression functions were added which all generate kibanaFilter type.

filtersToAst utility function was added which takes a Filter or array of them and returns array of expressions

kibana_context expression function was updated to take kibanaFilter type in as filters argument and all the places using it were updated.

resolves #67896

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ppisljar ppisljar requested a review from a team March 9, 2021 10:53
@ppisljar ppisljar requested a review from a team as a code owner March 9, 2021 10:53
@ppisljar ppisljar added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppServices v7.13.0 v8.0.0 WIP Work in progress labels Mar 9, 2021
@elasticmachine
Copy link
Contributor

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

@ppisljar ppisljar added the release_note:skip Skip the PR/issue when compiling release notes label Mar 9, 2021
@ppisljar ppisljar force-pushed the expressions/filter_functions branch 2 times, most recently from 4895426 to 4133707 Compare March 16, 2021 10:50
@ppisljar ppisljar force-pushed the expressions/filter_functions branch from 4133707 to 14a0237 Compare March 17, 2021 16:03
@kibanamachine
Copy link
Contributor

⏳ Build in-progress, with failures

Failed CI Steps

History

  • 💔 Build #113685 failed 14a02378bd2d4754419f45bd23e28ee290b20e0f
  • 💔 Build #113329 failed 4133707b70198da6284766735235c51075f60a15
  • 💔 Build #113198 failed 4895426f310d22b9b0ac6189d2ae5d5fa75587e9
  • 💔 Build #112438 failed 3a6ed1775938bba47b9e0a3a1c0ed968182d933e
  • 💔 Build #112421 failed 7c529f5ce1e6faa7b345dbc4a72b72fdd834b967

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

@ppisljar ppisljar force-pushed the expressions/filter_functions branch from ea2152d to a09746b Compare March 18, 2021 06:55
@ppisljar ppisljar force-pushed the expressions/filter_functions branch from a09746b to 3daaa59 Compare March 22, 2021 14:17
@ppisljar ppisljar added review and removed WIP Work in progress labels Mar 23, 2021
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, except the lt, lte, gt and gte checks, see below.

});

it('returns an object with the correct structure', () => {
const actual = fn(null, { field: { spec: { name: 'test' } } }, context);
Copy link
Contributor

@streamich streamich Mar 23, 2021

Choose a reason for hiding this comment

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

I understand this is copy-paste from similar tests in this plugin, but I've seen it a lot and wanted to suggest a small improvement. It is better to not create global module variables, like context here and not use beforeEach utility, which is specific to Jest. This can be done like so:

Suggested change
const actual = fn(null, { field: { spec: { name: 'test' } } }, context);
const context = setup();
const actual = fn(null, { field: { spec: { name: 'test' } } }, context);

Simply setup() function that creates everything needed for each test case every time.

src/plugins/data/common/search/expressions/field.ts Outdated Show resolved Hide resolved
src/plugins/data/common/search/expressions/field.ts Outdated Show resolved Hide resolved
};
});

it('returns an object with the correct structure', () => {
Copy link
Contributor

@streamich streamich Mar 23, 2021

Choose a reason for hiding this comment

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

Same here, fn and context are defined outside of the text case, and the test case name just says that something should work "correctly", but it is not clear what should be correct, and it is not clear why fn and context are coming from out of the test case.

src/plugins/data/common/search/expressions/range.ts Outdated Show resolved Hide resolved
src/plugins/data/common/search/expressions/range.ts Outdated Show resolved Hide resolved
src/plugins/data/common/search/expressions/range.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Kibana app code changes LGTM!

Copy link
Contributor

@dokmic dokmic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 616 623 +7
expressions 149 151 +2
total +9

Async chunks

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

id before after diff
data 202.1KB 202.1KB -2.0B

Page load bundle

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

id before after diff
data 772.2KB 780.3KB +8.1KB
expressions 192.6KB 193.3KB +798.0B
visualizations 107.3KB 107.4KB +72.0B
total +9.0KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes review v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data.query.filters] Create expression function for filters & add toAst method
6 participants