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

Add support for context aware autocompletion #171

Merged
merged 17 commits into from
Aug 24, 2022
Merged

Add support for context aware autocompletion #171

merged 17 commits into from
Aug 24, 2022

Conversation

sunker
Copy link
Contributor

@sunker sunker commented Aug 17, 2022

This PR switches from using the CodeEditor that was used in the query editor, variable editor and annotation editor with the SQL CodeEditor from grafana/experimental. This adds autocompletion including suggestions for macros, functions, keywords, logical operators, comparison operators etc. Like before, athena uses standard SQL so no specific language components are being passed to the SQL CodeEditor. However, a custom completion item provider is used so that the plugin can resolve tables and columns used by the autocompletion engine.

athenaexample

Please note that the variable editor is now rendering the full QueryEditor, including the left form group with resource selector. This should be fine I think. Please add feedback here if any.

Fixes #111

@CLAassistant
Copy link

CLAassistant commented Aug 17, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

Backend code coverage report for PR #171
No changes

@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2022

Frontend code coverage report for PR #171

Plugin Main PR Difference
src 87.01% 83.93% -3.08%

.type(
`{selectall}
SELECT $__parseTime(eventtime, 'yyyy-MM-dd''T''HH:mm:ss''Z'), sum(cast(json_extract_scalar(additionaleventdata, '$.bytesTransferredOut') as real)) AS bytes
FROM $__table WHERE additionaleventdata IS NOT NULL AND json_extract_scalar(additionaleventdata, '$.bytesTransferredOut') IS NOT NULL AND $__timeFilter(eventtime, 'yyyy-MM-dd''T''HH:mm:ss''Z')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to adjust formatting slightly here so that suggestions are not accepted when hitting enter to get a newline. :)

@sunker sunker marked this pull request as ready for review August 18, 2022 13:19
@sunker sunker requested a review from a team as a code owner August 18, 2022 13:19
@sunker sunker requested review from iwysiu, kevinwcyu and andresmgot and removed request for a team August 18, 2022 13:19
@andresmgot
Copy link
Collaborator

Please note that the variable editor is now rendering the full QueryEditor, including the left form group with resource selector. This should be fine I think. Please add feedback here if any.

The editor was different because the format selector does not make sense in the variable editor, it should always be "Table". It's a, maybe minor, UX issue.

Copy link
Collaborator

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

👍 The autocompletion works flawlessly, nice!

When trying this I noticed a bug though (but it seems that it's not related to this PR). It seems that setting the catalog, database or column to the "default" is not working anymore. This is the curated dashboard:

Screenshot from 2022-08-19 09-46-28

And this is a fresh new panel:

Screenshot from 2022-08-19 09-50-53

Feel free to ignore that comment and address it in a different issue if it's not related!

package.json Outdated
@@ -18,6 +18,7 @@
"author": "Grafana Labs",
"license": "Apache-2.0",
"devDependencies": {
"@grafana/experimental": "^0.0.2-canary.38",
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if Athena is run with a lower version of Grafana? (that does not include the latest experimental package)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good shout, better make this a dependency. I wonder what would happen though if grafana is running a version of the experimental package that is lower than the plugin specifies..?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I belive it gets ignored, no matter if it's a dependency or a devdep (there is no reference to the deps in the dist folder, when you build a plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fine since experimental package is not specified as an external dep in core grafana

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I didn't know it works that way, interesting!

@sunker sunker changed the title Use completion item provider from grafana experimental Add support for context aware autocompletion Aug 19, 2022
@sunker
Copy link
Contributor Author

sunker commented Aug 19, 2022

The editor was different because the format selector does not make sense in the variable editor, it should always be "Table". It's a, maybe minor, UX issue.

Yep true. It default to table, but I made it possible to opt out on these options in the query editor so they are now no longer visible in the variable editor.

Not sure if I'm doing something wrong here but I can't repro the bug. Thoughts?
athenabug

@andresmgot
Copy link
Collaborator

Not sure if I'm doing something wrong here but I can't repro the bug. Thoughts?

sorry! it may be that I had old deps installed, after reinstalling everything I don't find that issue

@sunker sunker added effort/large enhancement New feature or request and removed effort/large labels Aug 22, 2022
// Verify editor suggestions
e2eSelectors.QueryEditor.CodeEditor.container().click({ force: true }).type(`{selectall}$__table`);
e2eSelectors.QueryEditor.CodeEditor.container().contains('(Macro) cloudtrail_logs');
// The follwing section will verify that autocompletion in behaving as expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The follwing section will verify that autocompletion in behaving as expected.
// The following section will verify that autocompletion in behaving as expected.

description: 'Will return the current ending time of the time range. Use second argument to specify time format.',
},
{
id: '$__unixEpochFilter()',
Copy link
Contributor

Choose a reason for hiding this comment

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

A reminder about the macro corrections for this pr

return (
<SQLCodeEditor
query={query.rawSQL}
onBlur={() => onRunQuery()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that this can't just be onBlur={onRunQuery}?

@sunker sunker merged commit cc836cc into main Aug 24, 2022
@sunker sunker deleted the autocomplete branch August 24, 2022 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use code editor autocomplete provider from grafana/experimental
4 participants