[Discover] Supports SQL query language (#134429) #136702
Conversation
* Move the add dataview action above the dataview selection panel * Implements a new selectable on the dataview picker for the text based languages * Implementation of the transition modal when on SQL mode and select a dataview * Fix es lint * Change switch modal button modal icon * Lazy load components * Small changes on the styling of the switch without saving button * Initialization of mocaco editor * Change to the type * Fixes types checks * New submit button for query mode * Implememtation of the expanded mode of the editor * Implement documentation * Implementation of the oneliner mode with ellipsis * Some fixes on the resizer * Implementation of the errors layout, WIP * Fetch SQL data in Discover * Fix expression test * Fix editor zIndex * Fix types error * Fix type check in Discover * Fix more types * some CI fixes * Fixes * Cleanup after merge * Remove from state * Connect search errors with the unified search editor * Add error mrkers in unified search editor * Save and open saved searches * Filter out saved searches from text based languages * Some fixes * Fix unit tests * Fix checks * On save and exit modal implementation * Add shortcut on the editor for submit query * Fix wrong condition * Initial types change * Use regex to find the index pattern string * Fix some types and cleanup * Fix types * Fix some types * Further fixes * More fixes * More fixes * Fix visualize types * more * More fixes * Fixes more types * Fix dashboard types * Fix dashboard types * Controls plugin types * Fix Lens types * Fix data plugin types * Fix types in Lens 2 * buildEsConfig type fixes * Fix observability types * Fix maps types * data visualizer types * Fix ml types * xpack rest types * Fix jest test * Fix * Move helper functions to es config * fix bug on breadcrumb click * Fix time field bug * Add enableSql advanced setting to discover for enabling the sql mode * Make the documentation component more dynamic * Add some comments, improvements * Enhance storybook with the textbased languages * Update storybook with the error state of the editor * Adds a readme for the editor and fixes the modal mobile version * [Discover] improve test and storybook for new data type * [Discover] add functional tests * Add aggregate functions to the documentation * [Discover] fix tests * Add some unit tests * [Discover] fix linting * [Discover] update linting * More unti tests * Dataview picker unit tests * Fix a bug on the dataview picker * Add unit tests for the editor * Fix jest test * [Discover] apply suggestions * [Discover] adjust styles * Fix some bugs and select columns in the sql mode * [Discover] fix eslint and tests * [Discover] update unit tests * Fix bug on transitioning from sql mode to dataview mode * [Discover] fix tests * Design fixes on the errors messages * [Discover] fix ci * Update the columns only if the query changes * [Discover] change isPlainRecord retrieval method * Fix bug on cleanup * Fix bug on opening a saved search * [Discover] fix comments * [Discover] fix bug with browser refresh * [Discover] fix functional * [Discover] fix another functional * Fix ordering lost when the user refreshes the browser * [Discover] revert use_discover_state * [Discover] revert functional impl * Fix security solution types * Casting dashboard plugin * Revert change * type param * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Revert types changes * More reverts * Types fixes * Fix Discover jest test * Fix context app jest test * Final types changes * Fixes unit test Co-authored-by: Dzmitry Tamashevich <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Joe Reuter <[email protected]>
stratoula
added
Feature:Unified search
|
I am currently working on these design improvements #134429 (review) |
stratoula
added
the
Team:DataDiscovery
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
stratoula
added
the
Team:Visualizations
|
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
|
@MichaelMarcialis I have addressed the majority of your comments except from:
I can't replicate this. Can you give me the query that you are using?
I didn't implement it as we can only have one error and not a list of errors. So the user can't be confused with it. Focusing on a specific line of the monaco editor is also something that needs investigation. I can add it on the list for 8.5.
I used the current Discover logic for displaying errors. I can look into it on a follow up PR! Depending on my comments above and the fact that this feature is hidden under an advanced setting, do we consider them as blockers for merging this PR? |
src/plugins/discover/public/application/main/components/sidebar/discover_field.tsx
Outdated
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
VisEditors changes look mostly good to me, noticed two small things but they don't block the PR:
Data view recognition is using the first FROM from the query, but in case of nested selects this doesn't work:
It's possible to save a saved search, build a vis on it, then switch it to sql and save it again. The panel then fails like this:
This could be fixed by "forgetting" the current saved search when switching to/from SQL mode.
Approving as I'm happy to merge as-is
|
Thanx @flash1293! I am moving them to the follow up issue! |
|
Fixed the nested selects problem (wrong dataview recognition)
|
There was a problem hiding this comment.
Thanks for making those additional requested changes, @stratoula. Amazing work! I've put together an updated list of comments for your review below.
I know feature freeze is looming and we're eager to get this feature in. At the end of the day, none of my comments are what I'd consider hard blockers, so I'm going to go ahead and approve now so I don't hold you up further (under the assumption that these either get addressed in this PR or spun off as a separate issue/PR).
Replies
About the resizer I am fine on copying the styles but it is a bit confusing for me as I am not very comfortable with the EUI code. I would need some help here.
Certainly. Do you want my assistance, or are you looking for one of the experts from the @elastic/eui-design team? If me, feel free to throw something on the calendar for us to hop on a Zoom.
New Comments
- For the error popover, would it be possible to put an
alignItems="flexStart"prop on the containingEuiFlexGroup? Doing so should prevent the first column (with icon and line number) from being vertical aligned to the center (noticeable when the message breaks a line).
- Would it be possible to support word breaking (via the
euiTextBreakWordmixin, I believe) for the messages within the error popover? Currently, long strings of unbroken text can overflow out of the popover.
- For the unfocused editor in compact mode, the presence of the invalid red bottom border appears to be throwing off the editor's alignment (by increasing it's height by 1px) relative to other adjacent elements. Can this be remedied? If the custom styles currently in place aren't working, one alternative may be to use styles like EUI does for standard input (where both the focus and invalid colored borders are created via a background gradient).
- The editor footer in expanded mode still appears to be misaligned by 1px horizontally.
- During a resize action, the gray border above the editor footer disappears and a blue border appears below/behind the footer. Can we correct this so that the gray border doesn't disappear and the blue border doesn't appear during a resize action?
- During a resize action, if an invalid border is present on the editor, it temporarily turns from a 2px border to a 1px border. After stopping the resize action, it correctly returns to 2px after a few seconds. Possible to fix?
-
It appears that field type filtering is being omitted when in SQL mode. Is this intentional?
-
It appears that field type tokens and tooltips are missing from the field list in SQL mode. Is this intentional?
-
Interacting with one or more "Remove field from table" buttons in the field list doesn't appear to alter the SQL query. For example, if the user's query selects all fields (via
SELECT *), removing a field from the table keeps the wildcard (*) intact, which I found somewhat confusing. As a user, I'd expect the query to break out the wildcard into only the selected fields (i.e. all but the ones the user has chose to remove from the table). Is the current behavior intentional? Also worth noting with this currently implemented behavior is that refreshing doesn't restore all fields to the table, even though the wildcard is still present in theSELECTclause.
src/plugins/discover/public/components/discover_grid/discover_grid.scss
Outdated
Show resolved
Hide resolved
src/plugins/unified_search/public/dataview_picker/text_languages_transition_modal.tsx
Outdated
Show resolved
Hide resolved
src/plugins/unified_search/public/dataview_picker/text_languages_transition_modal.tsx
Outdated
Show resolved
Hide resolved
good point, Michael, yes, this is something to discuss. In SQL you can do the following:
In this case
This is simply a limitation how Discover currently works , there is the data layer and the presentation layer, when you're adding/removing fields you're changing the presentation of your results and not the query. But it's not written in stone of course. |
|
Thanx @MichaelMarcialis, I really appreciate it! I addressed everything except from the 2 issues that have to do with the resizer. I added them to the followup issue #136950. This has something to do with monaco height update so I need some time to investigate it and find alternatives. |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
| @@ -45,14 +45,15 @@ export const createTileMapFn = (): TileMapExpressionFunctionDefinition => ({ | |||
| }, | |||
| }, | |||
| async fn(input, args) { | |||
| const query = input.query as Query; | |||
There was a problem hiding this comment.
how can we be sure that what's passed in here is not an AggregateQuery?
| } | ||
|
|
||
| const savedSearch = embeddable.getSavedSearch(); | ||
| const query = savedSearch.searchSource.getField('query'); |
There was a problem hiding this comment.
is it not possible that searchsource contains the query of type sql but it gets dropped (ignored) in the actual embeddable?
| return false; | ||
| } | ||
|
|
||
| const savedSearch = embeddable.getSavedSearch(); |
There was a problem hiding this comment.
can we be sure here that embeddable contains a saved search ?
| @@ -51,15 +51,15 @@ export const useEditorUpdates = ( | |||
| uiState: vis.uiState, | |||
| timeRange: timefilter.getTime(), | |||
| filters: filterManager.getFilters(), | |||
| query: queryString.getQuery(), | |||
| query: queryString.getQuery() as Query, | |||
There was a problem hiding this comment.
can we be sure this is not an AggregateQuery ?
Summary
This PR enables the SQL queries in Discover. The feature is available after an advanced setting is enabled.
The user can run sql queries and also save them as saved searches. These searches cant be used for creating an aggregation based visualization but can be imported on a dashboard.
For the technical preview we are going to support the following features:
Checklist
Delete any items that are not applicable to this PR.