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 SQL in prepration for SQLFluff v2 #3364

Closed
wants to merge 1 commit into from
Closed

Conversation

tunetheweb
Copy link
Member

SQLFluff v2 is more strict about whitespace and has just been released.

We'll wait for the GitHub super linter to support this, but this PR makes all the whitespace changes that are backwards compatible in preparation.

I also prefer this formatting anyway so think it's a good change.

@rviscomi
Copy link
Member

Not a fan of this style TBH. My preference is K&R for blocks in all languages (SQL, CSS, JS, etc).

@tunetheweb
Copy link
Member Author

Can't say I agree for SQL, but willing to stick with that.

Only thing is in that case we'll need to wait and do the changes once SQLFluff v2 is out in the Super Linter we use is CI and do the big change then (though should be a bit less big than this) as this style is the only way to be compatible with SQLFluff v1 and v2.

Will close this for now and look then.

@tunetheweb tunetheweb closed this Mar 17, 2023
@rviscomi
Copy link
Member

this style is the only way to be compatible with SQLFluff v1 and v2

This seems backwards; we shouldn't change our preferred style to conform to the linter rules, we should change the linter rules to conform to our preferred style. Or find a linter that's more compatible.

@tunetheweb
Copy link
Member Author

Well I preferred that style, which is why I've thought we could change it now, in preparation. But appreciate you don't.

So we can keep the current style. Just need to wait for v2 to be rolled out to the GitHub Super Linter we use in CI. There will be some changes then, but not getting rid of the initial open bracket from the first line. We just can't do those changes now as they are incompatible with SQLFluff v1 when opening bracket is still on first line, so can't keep both happy with that style.

So we'll wait for v2 to be used in the GitHub Super Linter, and then will open a PR for the changes needed then. Which hopefully will be one's you're OK with.

@tunetheweb
Copy link
Member Author

tunetheweb commented Mar 17, 2023

BTW was trying to figure out what I didn't like about the current style. And think it's because the parenthesis in FROM subqueries are groupings rather than control blocks.

You wouldn't use them in other clauses like the WHERE clause:

SELECT
  *
FROM
  table1
WHERE (
  col1 = 'abc' AND col2 = 123
) OR (
  col1 = 'xyz' AND col2 = 456
)

I would write that as this:

SELECT
  *
FROM
  table1
WHERE
  (col1 = 'abc' AND col2 = 123)
  OR
  (col1 = 'xyz' AND col2 = 456)

This more easily show it's part of the WHERE clause.

So I guess I see the parenthesis as more a grouping of the table name name, then a control block of the FROM clause. So I think of it more like this:

SELECT
  *
FROM
  (SELECT...)

When then often gets reformatted to this for a more complex subquery:

SELECT
  *
FROM
  (
    SELECT...
  )

But I do agree it's kinda verbose.

This also allows multiple tables being listed in the FROM clause, and both to be indented to show it's part of FROM clause:

SELECT
  *
FROM
  (SELECT...) t1,
  (SELECT...) t2
WHERE
  t1.id = t2.id

Our style isn't really to use above (except for UNNEST clauses) and we prefer to use JOIN clauses:

SELECT
  *
FROM
  (SELECT...) t1
JOIN
  (SELECT...) t2
USING (id)

So we usually only have one table per FROM clause, which allows us to get away with the current preference of treating the parenthesis as control blocks rather than a grouping of a single table name.

But anyway, I agree our current style is more inline with your preference so happy to keep that. But just wanted to clarify my thinking here (even if only to get it clear in my own head!).

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