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

Fix AST parsing when switching back to query builder #1002

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

SpencerTorres
Copy link
Collaborator

The AST module we use for parsing SQL is still not fully compatible with ClickHouse SQL, but there are things we can fix.

The button to switch from "SQL" to "Query Builder" has not been working as expected the past few releases, this aims to fix and enhance its capabilities.

old button (dramatized):
broken query builder button

new behavior (query is actually somewhat parsed correctly, columns are put into their associated boxes):
slightly better query builder button

I have added some logic to help properly map log/trace column names to their correct boxes based on the SQL column names/aliases.

Changes:

  • Switching from SQL editor to Query Builder is statistically less likely to destroy your query
  • SQL columns will be matched (best effort) to the correlated column selection box. This uses the default column mappings from the datasource config, including OTel if enabled.
  • Added tests
  • Changelog

Other notes:

  • The AST does not like backticks for column names
  • The AST will refuse to parse map keys with bracket syntax (ResourceAttributes['host'])
  • You must use quotes for your column name to preserve the case ("Timestamp"), otherwise it will be forced to lowercase. This behavior comes from the postgres AST module.
  • The ORDER BY should also used a quoted identifier, else it will try to use the lowercased identifier and likely throw an error when executing the query.
  • I have tried parsing ClickHouse's native EXPLAIN AST, but this does not include the required information to rebuild the query. Perhaps a hybrid approach would work?

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

Amazing work on this @SpencerTorres, especially with the testing! Thank you!

@aangelisc aangelisc merged commit 9bd5237 into grafana:main Oct 1, 2024
15 checks passed
@SpencerTorres SpencerTorres deleted the enhance-ast-parsing branch October 2, 2024 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants