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

[ES|QL] [Discover] Creating where clause filters from the table, sidebar and table row viewer #181399

Merged
merged 35 commits into from
May 6, 2024

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Apr 23, 2024

Summary

Part of #181280

This PR handles the creation of filters (where clause) in Discover ES|QL mode. The parts that are being handled are:

  • sidebar
  • document viewer
  • table

meow

The creation of filters from the charts is not here.

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula changed the title [ES|QL] Propagate the query with where clasue [ES|QL] Propagate the query with where clause Apr 23, 2024
@stratoula stratoula changed the title [ES|QL] Propagate the query with where clause [ES|QL] [Discover] Propagate the query with where clause Apr 25, 2024
@stratoula
Copy link
Contributor Author

/ci

@stratoula stratoula added backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana v8.15.0 Feature:Discover Discover Application labels Apr 29, 2024
@stratoula stratoula marked this pull request as ready for review April 29, 2024 13:18
@stratoula stratoula requested review from a team as code owners April 29, 2024 13:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

@stratoula stratoula changed the title [ES|QL] [Discover] Propagate the query with where clause [ES|QL] [Discover] Creating where clause filters from the table, sidebar and table row viewer Apr 30, 2024
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Very nice feature!

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

Overall working nicely, but I think there are some corner cases to look at

@stratoula
Copy link
Contributor Author

/ci

Copy link
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

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

ES|QL stuff looks good to me. I created a couple issues to improve our query manipulation DX

@drewdaemon
Copy link
Contributor

Lol... I guess I technically approved the whole thing since I'm a member of both Discover and ES|QL teams....

I only meant my review to be on behalf of the ES|QL team. I leave the Discover stuff to @jughosta

@stratoula
Copy link
Contributor Author

Thanx Drew, I want to revert and i will def not merge without Julia's approval :D

@stratoula
Copy link
Contributor Author

/ci

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

The functionality looks great!

One thing before the approval: can we update the following tests? Currently they generate incorrect schemas and might be less effective in testing:
packages/kbn-unified-data-table/src/components/data_table_columns.test.tsx

@stratoula
Copy link
Contributor Author

stratoula commented May 6, 2024

Ok so @jughosta on your comments:

  • simplified the code adc9d6e
  • fixed the tests that were not already correct in main fff036d (we were also marking extension as number which seems wrong to me, I changed it to string)
  • created an issue about the utility you suggested [DataViews] Create a utility to create a new ES|QL type field #182646. I think it needs a discussion first and should not be part of this PR
  • about the FT I get your way of thinking. I suggest to move on with one FT and if we feel it creates problems in the future we can split. I hope you are ok with it.

@stratoula
Copy link
Contributor Author

/ci

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Awesome work! And thanks for addressing the comments! 🚢

@stratoula
Copy link
Contributor Author

/ci

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/esql-utils 28 34 +6
@kbn/unified-data-table 83 80 -3
total +3

Async chunks

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

id before after diff
aiops 422.6KB 422.6KB +9.0B
apm 3.2MB 3.2MB +14.0B
cloudSecurityPosture 452.2KB 452.6KB +398.0B
dataVisualizer 656.1KB 656.1KB +6.0B
discover 629.3KB 631.0KB +1.8KB
lens 1.4MB 1.4MB +6.0B
logsExplorer 317.8KB 317.8KB +14.0B
ml 4.1MB 4.1MB +7.0B
securitySolution 13.7MB 13.7MB +1.9KB
slo 764.0KB 764.4KB +407.0B
unifiedHistogram 73.2KB 73.2KB +21.0B
total +4.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/unified-doc-viewer 6 5 -1

Page load bundle

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

id before after diff
aiops 7.3KB 7.3KB +54.0B
apm 34.6KB 34.6KB +54.0B
cloudSecurityPosture 16.0KB 16.1KB +139.0B
logsExplorer 54.3KB 54.4KB +139.0B
ml 77.2KB 77.2KB +54.0B
slo 23.0KB 23.1KB +54.0B
total +494.0B
Unknown metric groups

API count

id before after diff
@kbn/esql-utils 30 36 +6
@kbn/unified-data-table 152 149 -3
total +3

History

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

@stratoula stratoula merged commit b64500b into elastic:main May 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application Feature:ES|QL ES|QL related features in Kibana release_note:feature Makes this part of the condensed release notes Team:ESQL ES|QL related features in Kibana v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants