sql-formatter icon indicating copy to clipboard operation
sql-formatter copied to clipboard

[FORMATTING] comments after final WITH statement

Open BFJonk opened this issue 1 year ago • 3 comments

Input data

Which SQL and options did you provide as input?

/* fix late arriving client dimension keys */
     with result as (
          -- do the update
             update star.fact_user fu
                set dim_client_key = dim_client_hist.dim_client_key
               from star.dim_client_hist
              where
                    -- match business keys
                    fu.client_id = dim_client_hist.id
                    -- pick correct version
                and fu.date between dim_client_hist.dss_start_date and dim_client_hist.dss_end_date
                    -- only update these records
                and fu.dim_client_key = 0
                and fu.client_id is not null
          returning 1
          )
        , updated_rows as (
          -- count the updated records
             select count(*) as count
               from result
          )
          -- return success message   
   select 1 as p_status
        , (
          'Updated ' || updated_rows.count || ' records from star.fact_user with late arriving dimensional keys from star.dim_client_hist'
          )::varchar as p_return_msg
     from updated_rows
;

Expected Output I expected the comment in line 22 to be aligned with the select afte the comment

/* fix late arriving client dimension keys */
     with result as (
          -- do the update
             update star.fact_user fu
                set dim_client_key = dim_client_hist.dim_client_key
               from star.dim_client_hist
              where
                    -- match business keys
                    fu.client_id = dim_client_hist.id
                    -- pick correct version
                and fu.date between dim_client_hist.dss_start_date and dim_client_hist.dss_end_date
                    -- only update these records
                and fu.dim_client_key = 0
                and fu.client_id is not null
          returning 1
          )
        , updated_rows as (
          -- count the updated records
             select count(*) as count
               from result
          )
-- return success message   
   select 1 as p_status
        , (
          'Updated ' || updated_rows.count || ' records from star.fact_user with late arriving dimensional keys from star.dim_client_hist'
          )::varchar as p_return_msg
     from updated_rows
;

NB. Not sure if this will start to cause other problems if you manage to fix this PS. the as statements (aliases) in line 23 & line 26 are also not aligned

Actual Output

/* fix late arriving client dimension keys */
     with result as (
          -- do the update
             update star.fact_user fu
                set dim_client_key = dim_client_hist.dim_client_key
               from star.dim_client_hist
              where
                    -- match business keys
                    fu.client_id = dim_client_hist.id
                    -- pick correct version
                and fu.date between dim_client_hist.dss_start_date and dim_client_hist.dss_end_date
                    -- only update these records
                and fu.dim_client_key = 0
                and fu.client_id is not null
          returning 1
          )
        , updated_rows as (
          -- count the updated records
             select count(*) as count
               from result
          )
          -- return success message   
   select 1 as p_status
        , (
          'Updated ' || updated_rows.count || ' records from star.fact_user with late arriving dimensional keys from star.dim_client_hist'
          )::varchar as p_return_msg
     from updated_rows

Usage

  • How are you calling / using the library?

image

  • What SQL language(s) does this apply to? At least PostgreSQL

  • Which SQL Formatter version are you using? Online version image

BFJonk avatar Nov 09 '23 11:11 BFJonk

Thanks for reporting. Ignoring all the irrelevant SQL code, this issue comes down to:

select col
-- comment
from tbl;

getting formatted as:

select
  col
  -- comment
from
  tbl;

instead of:

select
  col
-- comment
from
  tbl;

This is indeed unfortunate. It happens because the formatter really only looks at the tokens that preceded the comment, it's not really aware of the code that's following. Implementing this sort of lookahead is possible... but it's a major undertaking. Unlikely to happen. Instead I'm concentrating on implementing an entirely new SQL formatting tool, which should solve most of the shortcomings of the SQL Formatter.

Regarding the indentation of AS-keywords, this feature is known to be buggy and will likely be removed entirely in the future.

nene avatar Nov 09 '23 12:11 nene

Hi @nene / Rene,

Thanks for answering. Nice you're working on a new version. Unfortunate that a lookahead is difficult. Few questions:

  • The first link has no options yet
  • It also has no PostgreSQL variant yet

I guess these are still to come :-)

  • The second link leads to a 404
  • What do you mean by, 'the indentation of AS-keywords will be removed'? Will the AS keyword lead to a syntax error?

BFJonk avatar Nov 20 '23 11:11 BFJonk

The first link has no options yet

There are a few options. Like the option to uppercase keywords. (Not accessible from the demo page though). But the goal with this tool is to have a bare minimum of formatting options. Following more in the footsteps of the Prettier formatting tool.

It also has no PostgreSQL variant yet

Yep, that's still in the works, because I first need to implement a parser for PostgreSQL.

The second link leads to a 404 What do you mean by, 'the indentation of AS-keywords will be removed'? Will the AS keyword lead to a syntax error?

Sorry about that. I already dropped this feature in master so the link was broken as a result. Fixed it to point at 13.x version.

The AS-keyword can be used just fine. Only the feature that indents AS keywords ("Align aliases" checkbox in your screenshot) has been removed.

nene avatar Nov 20 '23 13:11 nene