[ES|QL] Disable the filter actions for multivalue fields #193415
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stratoula
added
release_note:skip
|
Pinging @elastic/kibana-esql (Team:ESQL) |
|
I think it would be still helpful to pick at least the first value from the array and use that in |
|
@jughosta as the full text search comes soon I want to do this correctly. Only the first value doesnt return what they want so I dont think is a good decision. The users expect to find the documents that have the exact match of the multivalue and this will come with full text search and the QSTR function. When we will have this, we will have the same experience with the dataview mode. The |
|
Have a question, quickly testing it. First we should aim to to not show the filter icon if filtering is no possible (and it was already discussed in the issue, it shouldn't be a large effort checking for an array in the value, right @davismcphee @jughosta, but of course I might miss a 🐰 🕳️ ). So, if we can't add this to this PR (which aims to move fast 🏃 ), we are creating to a state where we currently don't give any user feedback what's happening, or how it could be fixed. So while I prefer toasts for breakfast, how about a toast here when it is clicked (however i struggle to find words, I must admit)? Or can we think of another user feedback preventing a non-functional button? |
davismcphee
reviewed
Sep 24, 2024
There was a problem hiding this comment.
While we can't fully hide the filter icons due to the way EUI data grid works, rather than silently ignoring the user's action, how about we disable the buttons and inform the user it's unsupported?
I opened a PR against this branch if we all agree with this approach: stratoula#35.
…es-esql [Discover] [Unified Data Table] Disable filtering for multivalue fields in ES|QL mode
|
Awesome @davismcphee 🙇♀️ |
|
/ci |
|
@davismcphee so happy nobody listend to me! Toast invasion prevented! |
kertal
reviewed
Sep 24, 2024
|
yes Matthias this makes sense, this is why I suggested to the issue to take it as a team on a follow up. We can do this on this PR of course as Davis has already worked on it but I would appreciate if you take over this. 🙏 We are going to revert these changes soon btw as QSTR is already on snapshot releases. This is why I wanted to so something quick. 
QSTR is going to be moved to Tech preview on 8.16 and on GA on 8.17 (most possibly). I want to disable it for now and enable it correctly for 8.17. So with that being said feel free to keep working on this PR. I would just like to disable it and fix the bug in 8.16 if possible 🙏 |
sure this makes sense!
I assume it's more work to also enable this for the DocViewer (with a quick look at the code), however I think we should be able to bring this over the finishing line before 8.16, wdyt @davismcphee ( |
davismcphee
approved these changes
Sep 24, 2024
There was a problem hiding this comment.
I agree with both of you @stratoula and @kertal 🙂 Let's merge this one as is and I'll open a followup to do the same for the doc viewer 👍
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
vadimkibana
approved these changes
Sep 25, 2024
logeekal
approved these changes
Sep 25, 2024
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 25, 2024
) ## Summary Part of elastic#193015 It not allows the creation of where clause filters in case of multi value fields as this is not supported yet in ES|QL. Check my comment here elastic#193015 (comment) It might be possible with full text search but I need to talk to the team first. For now we disable it as it creates a wrong filter. ### Checklist - [ ] [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 --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Davis McPhee <[email protected]> (cherry picked from commit 2492981)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
delanni
added a commit
that referenced
this pull request
Sep 25, 2024
delanni
added a commit
that referenced
this pull request
Sep 25, 2024
## Summary As the title says. Two competing changes made it in to main (#193415 & #193791), causing a type issue: https://buildkite.com/elastic/kibana-on-merge/builds/50786#019228ae-4487-45c6-867a-ba0590b1266d cc: @davismcphee @stratoula
kibanamachine
added a commit
that referenced
this pull request
Sep 25, 2024
…) (#193962) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Disable the filter actions for multivalue fields (#193415)](#193415) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Stratoula Kalafateli","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-25T10:14:34Z","message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [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\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Feature:ES|QL","Team:ESQL","v8.16.0"],"title":"[ES|QL] Disable the filter actions for multivalue fields","number":193415,"url":"https://github.com/elastic/kibana/pull/193415","mergeCommit":{"message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [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\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193415","number":193415,"mergeCommit":{"message":"[ES|QL] Disable the filter actions for multivalue fields (#193415)\n\n## Summary\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015\r\n\r\nIt not allows the creation of where clause filters in case of multi\r\nvalue fields as this is not supported yet in ES|QL. Check my comment\r\nhere\r\nhttps://github.com//issues/193015#issuecomment-2360704651\r\n\r\nIt might be possible with full text search but I need to talk to the\r\nteam first. For now we disable it as it creates a wrong filter.\r\n\r\n### Checklist\r\n\r\n- [ ] [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\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Davis McPhee <[email protected]>","sha":"249298166fe1499b4aea501056a2291711427c5b"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: Stratoula Kalafateli <[email protected]>
10 tasks
davismcphee
added a commit
that referenced
this pull request
Sep 27, 2024
…Viewer (#194232) ## Summary This PR disables ES|QL multivalue filtering in Unified Doc Viewer, similar to what we did for Unified Data Table in #193415:  Part of #193015. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 27, 2024
…Viewer (elastic#194232) ## Summary This PR disables ES|QL multivalue filtering in Unified Doc Viewer, similar to what we did for Unified Data Table in elastic#193415:  Part of elastic#193015. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [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 - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 6f01d6a)
kibanamachine
added a commit
that referenced
this pull request
Sep 27, 2024
…d Doc Viewer (#194232) (#194325) # Backport This will backport the following commits from `main` to `8.x`: - [[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)](#194232) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Davis McPhee","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T16:28:48Z","message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\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\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor","Feature:UnifiedDocViewer","Feature:ES|QL","Team:ESQL"],"title":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer","number":194232,"url":"https://github.com/elastic/kibana/pull/194232","mergeCommit":{"message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\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\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194232","number":194232,"mergeCommit":{"message":"[Discover] [ES|QL] Disable ES|QL multivalue filtering in Unified Doc Viewer (#194232)\n\n## Summary\r\n\r\nThis PR disables ES|QL multivalue filtering in Unified Doc Viewer,\r\nsimilar to what we did for Unified Data Table in #193415:\r\n\r\n\r\n\r\nPart of https://github.com/elastic/kibana/issues/193015.\r\n\r\n### Checklist\r\n\r\n- [x] Any text added follows [EUI's writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing), uses\r\nsentence case text and includes [i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\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\r\n- [ ] [Flaky Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was\r\nused on any tests changed\r\n- [x] Any UI touched in this PR is usable by keyboard only (learn more\r\nabout [keyboard accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [ ] If a plugin configuration key changed, check if it needs to be\r\nallowlisted in the cloud and added to the [docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n- [ ] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [ ] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"6f01d6a2605e69737ddff0993fc07d8ab8deb43e"}}]}] BACKPORT--> Co-authored-by: Davis McPhee <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Part of #193015
It not allows the creation of where clause filters in case of multi value fields as this is not supported yet in ES|QL. Check my comment here #193015 (comment)
It might be possible with full text search but I need to talk to the team first. For now we disable it as it creates a wrong filter.
Checklist