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

format native queries #38113

Merged
merged 4 commits into from
Jan 31, 2024
Merged

format native queries #38113

merged 4 commits into from
Jan 31, 2024

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented Jan 24, 2024

Closes #9142

Description

It is one of my projects for our internal hackathon. This PR adds format SQL query button.
Since it is a big package which is needed only for a single button, I put it in a separate chunk and import dynamically.
The button is hidden for nosql data sources such as Mongo, Druid, Google Analytics, and some sql databases such as sqlite and mssql for which optional clauses break formatting. For other SQL databases if the formatter library supports the specific dialect of given database it will use the dialcet otherwise it will fall back to ANSI SQL formatter.

How to verify

Ensure formatting works for SQL databases with the right dialect.
Ensure there is no formatting button for nosql databases.
Ensure the formatter library loads only when user clicks on the Format Query button.

Demo

sql-formatter.mov

Checklist

  • Tests have been added/updated to cover changes in this PR

@alxnddr alxnddr self-assigned this Jan 24, 2024
@metabase-bot metabase-bot bot added the .Team/DashViz Dashboard and Viz team label Jan 24, 2024
@alxnddr alxnddr added the no-backport Do not backport this PR to any branch label Jan 25, 2024
@alxnddr alxnddr requested review from a team January 26, 2024 17:51
@alxnddr alxnddr marked this pull request as ready for review January 26, 2024 17:51
Copy link

replay-io bot commented Jan 26, 2024

StatusComplete ↗︎
Commit8479e3f
Results
⚠️ 12 Flaky
2240 Passed

@@ -0,0 +1,53 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

How to verify

Ensure formatting works for SQL databases with the right dialect.
Ensure there is no formatting button for nosql databases.
Ensure the formatter library loads only when user clicks on the Format Query button.

These sound like very good test cases. Let's cover them!

keywordCase: "upper",
linesBetweenQueries: 2,
paramTypes: {
// Snippets, parameters, nested questions
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting does not work for queries with optional clauses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed for engines excluding mssql and sqlite due to the lack of support for it

@kamilmielnik kamilmielnik requested a review from a team January 29, 2024 08:11
Copy link
Contributor

@uladzimirdev uladzimirdev left a comment

Choose a reason for hiding this comment

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

LGTM overall :shipit:

Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

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

Awesome!

describe("scenarios > question > native", { tags: "@mongo" }, () => {
const MONGO_DB_NAME = "QA Mongo4";

before(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Let's either use beforeEach or let's lose the describe block and put everything inside a single it() block.

@@ -459,6 +459,49 @@ describe("no native access", { tags: ["@external", "@quarantine"] }, () => {
});
});

describe("scenarios > question > native", { tags: "@mongo" }, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this section related specifically to the code formatting?

Comment on lines +730 to +732
const query = question.query();
const engine = Lib.engine(query);
const queryText = Lib.rawNativeQuery(query);
Copy link
Member

Choose a reason for hiding this comment

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

<3

Copy link
Member

@nemanjaglumac nemanjaglumac left a comment

Choose a reason for hiding this comment

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

Left some inline (nit) comments but overall this is 🔥

@alxnddr alxnddr enabled auto-merge (squash) January 31, 2024 14:07
@alxnddr alxnddr merged commit 6657c3c into master Jan 31, 2024
108 checks passed
@alxnddr alxnddr deleted the format-sql-queries branch January 31, 2024 14:54
@paoliniluis
Copy link
Contributor

oh god 49 is gonna be so good

npfitz pushed a commit that referenced this pull request Feb 5, 2024
* format native queries

* optional clauses, mlv2, specs

* remove unnecessary webpack chunk name directive

* disable formatting for mssql and sqlite, address review comments
@a-escontrela
Copy link

Is it going to be available for BigQuery?
Is it possible to customize it by providing our own rules?

@alxnddr
Copy link
Member Author

alxnddr commented Feb 20, 2024

@a-escontrela yes, it will be available for BigQuery as well. Some custom rules may be added later on but are excluded from this version

@adeturckheim
Copy link

@alxnddr great feature! Big fan of it except for the default setup of a indent of 2 spaces (VS 4 spaces).
I guess it is a matter of personal tastes, but 4 is easier to read.

Only plan on making indent size configurable soon?


return formatSql(queryText, {
language: dialect,
tabWidth: 2,

Choose a reason for hiding this comment

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

Would love to be able to edit that one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Format SQL Query option in the Native Query Editor
7 participants