-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Console] Console with better SQL support #51446
[Console] Console with better SQL support #51446
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
💔 Build Failed |
Love the addition of SQL syntax highlighting! I am on the fence regarding the use of Github-style language identifiers. On one hand it requires users to learn a completely new syntax feature. On the other hand it would allow us to add support for additional syntaxes in the future (e.g. Painless, XML). I need to think about this some more. |
@cjcenizal I hear what you're saying re the introduction of the new syntax. The latest commit uses the Ace highlighter stack to determine when it's inside of a body where Less good thingsAt an implementation level I think this is a less good solution for the following reasons:
Good things
The """x markers route
|
💚 Build Succeeded |
@jloleysens Great analysis, I agree with all of your points. I'm starting to lean towards the language markers approach. Could we enhance it in the future with hardcoded rules that identify where SQL, Painless, etc are expected and logic that reformats the strings with the language marker automatically applied? The scenario I have in mind is one in which someone copies and pastes a request from a blog post or docs -- it's unlikely that this request will already be using the triple quoted strings and language markers, so it would be a nice assist to the user if they could have the appropriate syntax highlighting applied without having to reformat things manually. |
That sounds like a great idea for an enhancement and I think the scenario you are describing is going to happen very frequently! I'll keep some of the fixes to the use of templates from the latest commit and mix in the logic for language markers which we can bundle with the introduction of SQL syntax highlighting. |
- no flow :c
💚 Build Succeeded |
@elasticmachine merge upstream |
💔 Build Failed |
@elasticmachine merge upstream |
@pcsanwald Would you mind taking another look? |
Looks better, thank you! I wouldn't mind if the unsupported color was a little more differentiated, but would defer to style guide (do we have one for this sort of syntax highlighting?) One additional note: if we do add/remove SQL keywords, we'll need to keep our dialect highlighting up to date. I suppose the risk of adding/changing keywords is going to lessen over time, but I do want to be sure that we coordinate with SQL folks on the maintenance necessary going forward for adding this functionality. |
@pcsanwald in the case of I don't think it's the same as ANSII SQL's Left - might be mistaken. But getting some design feedback should definitely be a follow up PR imo.
I agree that we should keep a very close eye on the number of things we add to our plate that require manual checkup and updating. In this specific case I think we are fairly safe as changes to the SQL dialect are probably less frequent - @costin could you weigh in here? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, just had one question about some code that looks like it might be a no-op. I tested a bit locally but I'm not very familiar with our SQL API so I'd like to see if @costin could give it a whirl and see how he feels about the syntax highlighting.
@@ -54,6 +56,9 @@ export class UrlPatternMatcher { | |||
endpoint.methods.forEach((method) => { | |||
let c; | |||
let activeComponent = this[method].rootComponent; | |||
if (endpoint.template) { | |||
new FullRequestComponent(pattern + '[body]', activeComponent, endpoint.template); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but it doesn't look like we're assigning this to a variable, so what is this doing? I read through the constructors in the inheritance chain and didn't see any notable side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function call results in assignment in the component hierarchy:
Line 26 in 0eb4c18
parent.addComponent(this); |
SharedComponent
is the common ancestor to components like FullRequestComponent
. It's definitely not super clear at the moment. Perhaps a readme for how the autocomplete system works could be good? Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I see!
Perhaps a readme for how the autocomplete system works could be good?
This would certainly clear up a lot for me, but if it's time-consuming then I'm not sure it'd be worth it. Maybe we could begin with a brief outline of the roles of each component in the component hierarchy?
...lugins/console/public/quarantined/src/sense_editor/mode/elasticsearch_sql_highlight_rules.ts
Outdated
Show resolved
Hide resolved
I'll try to give it go by EoW. If there's a snapshot build of this branch, though, that would speed up the process. |
…le-sql-highlighting * 'master' of github.com:elastic/kibana: (56 commits) Migrate url shortener service (elastic#50896) Re-enable datemath in from/to canvas timelion args (elastic#52159) [Logs + Metrics UI] Remove eslint exceptions (elastic#50979) [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405) [ML] API integration tests - initial tests for bucket span estimator (elastic#52636) [Watcher] New Platform (NP) Migration (elastic#50908) Decouple Authorization subsystem from Legacy API. (elastic#52638) [APM] Fix some warnings logged in APM tests (elastic#52487) [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500) [ui/public/utils] Move items into ui/vis (elastic#52615) fix newlines in kbn-analytics build script Add top level examples folder and command to run, `--run-examples`. (elastic#52027) feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662) [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675) [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657) [SIEM][Detection Engine] Adds the default name space to the end of the signals index [Logs UI] Generalize ML module management (elastic#50662) Removing stateful saved object finder (elastic#52166) Shim oss telemetry (elastic#51168) [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344) ... # Conflicts: # src/legacy/core_plugins/console/public/legacy.ts # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts # src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts # src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
It's great to see progress on the UI side for SQL, thank you for doing this. Regarding the SQL highlighting, does it make sense to highlight other parts of a query, as well? For example, numbers to be in different color, same goes for string constants (text surrounded by single quotes ''). And colors to be a bit more differentiated? The snippet below comes from DBeaver |
@astefan Thanks for taking a look! I don't think I'm the best person for picking the final colors, but I did see there is a pallet of color overrides for ace so I just made brought the SQL highlighting (i.t.o. relative colours) more in line with the image you sent. Perhaps the best way to proceed would be to merge this work in (if we're happy with the current tokens that are being highlighted) and get design to do another pass where colours are tweaked. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
@jloleysens thanks again for this effort, it looks great! I'm leaving a few comments to potentially be picked up by next iteration. (Feel free to ignore any or all, I'm sure some of these have already been considered.)
|
Thanks for putting a smile on my face this morning @bpintea. 😄 |
Thanks for the feedback @bpintea! The highlighting you mentioned (cast and single vs double quote) was addressed in one of the last commits before merging :).
We can look into the other params! Our definitions may be outdated!
The completion behavior you noted is a tricksy one. The suggestions pop up as the cursor moves around but the expansions are “dumb” in the sense that they are just snippets. We can neaten this up for sure - it may be the case for all completions at the moment. Will have to take a look!
… On 13 Dec 2019, at 18:49, CJ Cenizal ***@***.***> wrote:
In my (irrelevant?) browser (Opera)
Thanks for putting a smile on my face this morning @bpintea. 😄
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
* First iteration of PoC for SQL highlighting and better completion * Alternate implementation (no """sql markers) * xLang markers * - Remove 'start-sql' - no flow :c * Revert "- Remove 'start-sql'" This reverts commit 2d585ee. * Revert "xLang markers" This reverts commit f019616. * Revert "xLang markers" and add comment This reverts commit f019616. * Add elasticsearch sql highlight rules * Add links to sources of data * Update colors * Redo built in functions # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/input_highlight_rules.js # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/x_json_highlight_rules.js
* First iteration of PoC for SQL highlighting and better completion * Alternate implementation (no """sql markers) * xLang markers * - Remove 'start-sql' - no flow :c * Revert "- Remove 'start-sql'" This reverts commit 2d585ee. * Revert "xLang markers" This reverts commit f019616. * Revert "xLang markers" and add comment This reverts commit f019616. * Add elasticsearch sql highlight rules * Add links to sources of data * Update colors * Redo built in functions # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/input_highlight_rules.js # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/x_json_highlight_rules.js
…52893 * '7.x' of github.com:elastic/kibana: [Console] Fix OSS build (elastic#53885) (elastic#54094) [Console] Console with better SQL support (elastic#51446) (elastic#54091) Fix suggested value for time_zone in range query (elastic#53841) (elastic#54092) [APM] Show errors on the timeline instead of under the transaction (elastic#53756) (elastic#54109) use NP deprecations in uiSettings (elastic#53755) (elastic#54009) adding message to transaction and span metadata (elastic#54017) (elastic#54090) # Conflicts: # x-pack/legacy/plugins/console_extensions/spec/overrides/sql.query.json
Summary
The idea behind this PoC is to investigate the amount of work it would take to up the SQL game in Console. This is quite rough around the edges still but could be a good way to get a conversation going around the best approach we could take for SQL in Console. Should it be a separate editor? Would we want to do the same thing with Painless (syntax highlighting supported)?Added support for SQL highlighting and better SQL request completion.
Things added:
SQL highlighting (note the addition of"""sql
syntax)POST _sql
endpointResources
Proposed syntax
Completion with sql markers
[UPDATE] Alternative, w/o SQL markers this is the option used in the end
Given that the addition of SQL markers introduces new syntax an alternative is to use the
_sql
endpoint as our marker for when to add SQL syntax highlighting.Updated w/o sql markers
Future work
It would be good to implement language markers in the future, this PoC demonstrated that it will not be too hard to do, we should just approach it as it's own unit of work. We'll need markers for SQL + Painless.
Release notes
Added improved autocomplete expansion for an entire request which is now available on the SQL endpoint in Console.
Added SQL syntax highlighting for the SQL endpoint in Console.