Issues with EAP 2018.2 formatter

Created June 29, 2018 20:27

Main issues I'm seeing initially:

1. IS NOT NULL gets a new line between NOT and NULL when there is a long inline comment following; this should not happen.

JOIN lkp.media_source ms ON ms.event_source = l.event_source
                            AND ms.media_source_hash = l.media_source_hash
                            AND ms.channel IS NOT
                            NULL -- blah comment blah comment blah comment blah comment blah comment blah comment blah comment blah comment

2. cannot disable: in ON join clause, newlined AND elements are aligned with the ON.

-- bad
SELECT *
FROM lkp.attribution_events l
JOIN lkp.media_source ms ON ms.event_source = l.event_source
                            AND ms.media_source_hash = l.media_source_hash
                            AND ms.channel IS NOT NULL

when you have alignment disabled, they should just get a continuation indent:

-- good
SELECT *
FROM lkp.attribution_events l
JOIN lkp.media_source ms ON ms.event_source = l.event_source
    AND ms.media_source_hash = l.media_source_hash
    AND ms.channel IS NOT NULL

3. I cannot prevent the ON from getting a new line when the ON condition is long. I have it Elements wrapping set to don't change, and Wrap ON disabled. We should be able to have ON on same line as JOIN.

4. Would like to see subqueries indented relative to start position of previous line, rather than relative to open parentheses position; i.e. as is done in java c# etc.

Bad:

SELECT *
FROM (
     SELECT *
     FROM tmp.test t
     ) a
LEFT JOIN (
          SELECT *
          FROM tmp.test t
          ) b ON 1 = 1

Good:

SELECT *
FROM (
    SELECT *
    FROM tmp.test t
) a
LEFT JOIN (
    SELECT *
    FROM tmp.test t
) b ON 1 = 1

5. It doesn't seem that we can control EXISTS(<subquery>) like we can with derived table i.e. SELECT * FROM (<subquery>).

E.g. instead of this:

WHERE NOT exists(SELECT
                     1
                 FROM tmp.model_events l
                 WHERE l.customer_id = me.customer_id AND l.event_period = 'blah');

Would like to see this:

WHERE NOT exists(
    SELECT 1
    FROM tmp.model_events l
    WHERE l.customer_id = me.customer_id AND l.event_period = 'blah'
);

6. missing setting: newline before WHEN in case statement

7. i have no wrapping set for case statement, but a long case statement is wrapped anyway. additionally, the wrapped line is is way too far to the right; it should just get a continuation indent, however it is instead indented relative to some odd rules depending on circumstance. as a consequence you get really odd results, e.g.:

CASE WHEN l.event_source IN ('blah1', 'blah2') THEN 1 WHEN l.event_source = 'blah3' AND l.event_type =
                                                            'view' THEN 1 WHEN l.event_source = 'blah4'
                                                                               AND l.event_type = 'blah5' AND
                                                                               l.is_last_event_in_class = 1

So everything is squished to the right, and more lines than needed. Just indent relative to start of prior line please in these continuations, and don't wrap if it's not requested.

3 comments

Leonid

Created July 02, 2018 18:55

Hello, thank you for the comments.

(1) It's a bug, will be fixed soon.

(2) Looks like we need an additional option — "Egyptian style code". I'll file an issue.

(3) Please, post an issue.

(4,5) Formatter works as designed. What you've named "bad" is really good in some styles. Please post your example as a feature request.

(6,7) Looks like bugs, will be fixed.

Dpstandish

Created July 10, 2018 15:47

Re (2), it seems like a bug to me. It shouldn't align unless you tell it to.

Re (3), DBE-6718 ON is wrapped even when "Wrap ON" is disabled.

Re (4,5):

Yes, code style is subjective; otherwise, there would be no need for settings. I have submitted a number of related requests concerning this issue:

In my experience a single indent is pretty standard. Anything more is alignment, which can be costly to achieve unless you are using a fancy editor like DG. I don't think the extra indentation is of much benefit with regard to intelligibility, and indenting more than necessary can produce some really horrendous-looking code when you have to use a few nested subqueries. But putting aside the question of the aesthetics or efficiency of it, if you have a project where you are collaborating with folks who don't have such an editor, it would be a really tough sell to ask them to code like that.

Re 5 specifically though, we can already control parentheses for subquery; shouldn't that apply to a subquery inside an exists?

Also, new issue:

8. sometimes elements are not chopped in SELECT even when chopping is enabled

E.g.

DROP TABLE IF EXISTS _non_brand_site_customers;
CREATE TEMPORARY TABLE _non_brand_site_customers AS
    SELECT customer_id
    FROM (
         SELECT s.customer_id, max(CASE WHEN channel <> 'Brand Site'
             THEN 1
                                   ELSE 0 END) AS has_non_brandsite
         FROM tmp.sessions s
         GROUP BY
             s.customer_id
         HAVING has_non_brandsite = 1
         ) a;

This also illustrates an issue with CASE WHEN formatting.

Other formatting issues that are collecting dust.

Dpstandish

Created July 24, 2018 22:42

@Leonid

Looked at 2018.2 RC 1.

1. "place clause elements" should not be under "common". it is very common to have each select element on its own line, and indented with 1 tab. to accomplish this we must set "place clause elements" to "new line". But because this is under "common", the tables in a from statement are treated the same way, which is highly unconventional, doesn't really make sense, and IMO is quite ugly. This setting should be taken out of common and set within each clause category.

1.b. if you set it to "do not change", even if you have it set to leave line breaks, it puts first select element inline, even if you have it set to 0 inline elements. and the indent of the select elements is bad. it does 7 characters but i have my tabs set to 4 characters. It should just be 1 tab!

2. the "indent join" setting does not work right. i have "indent join" set to no, but joins are indented anyway. and they are indented badly: 5 spaces instead of 4 (4 is what i have my indents set to); i guess it is aligning with "from" plus " ".

3. The ANDs in a join ON clause are out in the middle of nowhere. Can you please make it possible to just have them at 1 indent relative to start of prior line!

Bad formatting example for (1.a.):

CREATE TABLE IF NOT EXISTS tmp.sessions AS
    SELECT
        l.attribution_event_key,
        l.customer_id,
        l.event_source_id,
        f.is_first,
        f.visit_count,
        f.visit_seq,
        row_number() OVER (PARTITION BY l.customer_id, l.event_source, f.visit_count
            ORDER BY f.visit_seq DESC) AS rn,
        nvl(ch.channel, 'Brand Site') AS channel,
        nvl(ch.subchannel, 'Other') AS subchannel
    FROM
        lkp.attribution_events l
        JOIN lkp.media_source ms ON ms.event_source = l.event_source
                                        AND ms.media_source_hash = l.media_source_hash
                                        AND
                                    ms.channel IS NOT NULL -- ms.channel IS NOT NULL means "has been classified"
            -- todo: classify remaining media combinations
            --     AND ms.channel = 'Affiliate'
        LEFT JOIN table(udtf_visit_count(event_datetime)
                            OVER (PARTITION BY l.customer_id, l.event_source
                                ORDER BY l.event_datetime, l.attribution_event_key)) f ON 1 = 1
        LEFT JOIN table(udtf_visit_channel(ms.channel,
                                           ms.subchannel) OVER (PARTITION BY l.customer_id, l.event_source, f.visit_count
            ORDER BY l.event_datetime, l.attribution_event_key)) ch ON 1 = 1
    WHERE l.event_source = 'session';

Bad formatting example for (1.b.), (2.), and (3.):

SELECT l.attribution_event_key,
       l.customer_id,
       l.event_source_id,
       f.is_first,
       f.visit_count,
       f.visit_seq,
       row_number() OVER (PARTITION BY l.customer_id, l.event_source, f.visit_count
           ORDER BY f.visit_seq DESC) AS rn,
       nvl(ch.channel, 'Brand Site') AS channel,
       nvl(ch.subchannel, 'Other') AS subchannel
FROM lkp.attribution_events l
     JOIN lkp.media_source ms ON ms.event_source = l.event_source
                                     AND ms.media_source_hash = l.media_source_hash
                                     AND
                                 ms.channel IS NOT NULL -- ms.channel IS NOT NULL means "has been classified"
         -- todo: classify remaining media combinations
         --     AND ms.channel = 'Affiliate'
     LEFT JOIN table(udtf_visit_count(event_datetime)
                         OVER (PARTITION BY l.customer_id, l.event_source
                             ORDER BY l.event_datetime, l.attribution_event_key)) f ON 1 = 1
     LEFT JOIN table(udtf_visit_channel(ms.channel,
                                        ms.subchannel) OVER (PARTITION BY l.customer_id, l.event_source, f.visit_count
    ORDER BY l.event_datetime, l.attribution_event_key)) ch ON 1 = 1
WHERE l.event_source = 'session';

Please sign in to leave a comment.