Nested dict argument to jinja macro fails the formatter #471

rparvathaneni-sc · 2023-08-31T18:28:24Z

Describe the bug
The following config macro makes the formatter fail because of the nested dict (range in partition_by)

{{ config(
    materialized='table',
    partition_by={
      "field": "user_id",
      "data_type": "int64",
      "range": {
        "start": 0,
        "end": 100,
        "interval": 10
      }
    }
)}}

To Reproduce
Paste the macro above in an sql file and try to run the formatter

Expected behavior
Formatter not erroring out

Actual behavior

sqlfmt encountered an error: Closing bracket ')' found at 138 before bracket was opened.

Additional context
What is the output of sqlfmt --version?

sqlfmt, version 0.19.0

The text was updated successfully, but these errors were encountered:

tconbeer · 2023-08-31T22:19:28Z

I can't repro on main, v0.19.0, or sqlfmt.com.

(shandy-sqlfmt-BZvASPt6-py3.8) tco@bros-gold:~/open/sqlfmt$ sqlfmt -
{{ config(
    materialized='table',
    partition_by={
      "field": "user_id",
      "data_type": "int64",
      "range": {
        "start": 0,
        "end": 100,
        "interval": 10
      }
    }
)}}
{{
    config(
        materialized="table",
        partition_by={
            "field": "user_id",
            "data_type": "int64",
            "range": {"start": 0, "end": 100, "interval": 10},
        },
    )
}}

1 file formatted.
0 files left unchanged.

benjamin-awd · 2023-10-12T10:20:51Z

I've run into a similar issue -- plugging the code into sqlfmt.com works fine though, so I'm just manually replacing the buggy code

Original code:

{{
    config(
        materialized='incremental',
        incremental_strategy='insert_overwrite',
        external_location=generate_external_location("bucket", "datalake/xxx"),
        partitioned_by=['batch_date'],
        lf_tags_config={
          'enabled': true,
          'tags': {
            'domain': 'foo',
            'sensitivity': 'bar'
          }
        }
    )
}}

After formatting:

{{
    config(
        materialized="incremental",
        incremental_strategy="insert_overwrite",
        external_location=generate_external_location("bucket", "datalake/xxx"),
        partitioned_by=["batch_date"],
        lf_tags_config={
            "enabled": true,
            "tags": {"domain": "foo", "sensitivity": "bar"},
        },
    )
}}

tconbeer · 2023-10-12T16:29:12Z

Can you run pip list, please? I need to know what versions of shandy-sqlfmt and black you have installed.

benjamin-awd · 2023-10-13T07:56:36Z

I have a pretty long list of dependencies, but in my project I'm using

black                               22.12.0
shandy-sqlfmt                       0.20.0

Some additional debugging messages, in case it's useful:

# from node_manager.py
INFO:root:Token(type=TokenType.UNTERM_KEYWORD, token=select, spos=374)
INFO:root:Token(type=TokenType.UNTERM_KEYWORD, token=from, spos=1308)
INFO:root:Token(type=TokenType.UNTERM_KEYWORD, token=where, spos=1374)
INFO:root:Token(type=TokenType.BRACKET_CLOSE, token=), spos=310)
1 file had errors while formatting.
0 files left unchanged.
kp_dbt/dbt/app/models/kp_curated/web3/rpt_web3__subscription.sql
    sqlfmt encountered an error: Closing bracket ')' found at 310 before bracket was opened.

# from analyzer.py
-> rule.action(self, source_string, match)
(Pdb) rule
Rule(name='bracket_close', priority=510, pattern='(\\]|\\)|\\})', action=functools.partial(<function add_node_to_buffer at 0x106a6cee0>, token_type=<TokenType.BRACKET_CLOSE: 19>))
(Pdb) match
<re.Match object; span=(310, 315), match='    )'>

tconbeer · 2023-10-13T12:35:09Z

Thanks. Those logs are surprising -- the error is coming from pos 310, after later parts of the query are lexed... can you share the rest of your query (or another reproducible example), through the end of the where clause?

benjamin-awd · 2023-10-14T09:47:30Z

This query triggers sqlfmt encountered an error: Closing bracket ')' found at 330 before bracket was opened.

{{
    config(
        materialized='incremental',
        incremental_strategy='insert_overwrite',
        external_location=generate_external_location("baz", "datalake/producer=foo/object=bar"),
        partitioned_by=['batch_date'],
        lf_tags_config={
          'enabled': true,
          'tags': {
            'domain': 'foo',
            'sensitivity': 'public'
          }
        }
    )
}}

select
    batch_date
from
    {{ ref('bar') }}
{% if is_incremental() %}
where batch_date = '{{ var("ds") }}'
{% endif %}

The issue seems linked to the line-length argument. Running sqlfmt <path_to_query> --line-length 88 works fine, but sqlfmt <path_to_query> --line-length 120 triggers the error.

I've pinpointed the lack of a trailing comma as the issue -- if you add the comma before running sqlfmt, no errors are raised.

        lf_tags_config={
          'enabled': true,
          'tags': {
            'domain': 'foo',
            'sensitivity': 'public'
+          },
        }

tconbeer · 2023-10-15T20:33:54Z

I can reproduce on main, only with a line length of 95 or greater, which is very strange.

benjamin-awd · 2023-10-17T17:40:24Z

Looks like it's an issue with the search_for_terminating_token function

Starting var:.. start_rule = 'jinja_expression_start'
Starting var:.. program = re.compile('[^\\S\\n]*((\\{\\{-?)|(-?\\}\\}))', re.IGNORECASE|re.DOTALL)
Starting var:.. nesting_program = re.compile('[^\\S\\n]*(\\{\\{-?)', re.IGNORECASE|re.DOTALL)
Starting var:.. tail = ' config(lf_tags_config={"enabled": true, "tags": {"domain": "foo", "sensitivity": "public"}}) }}\n'
Starting var:.. pos = 2
00:52:32.838857 call       166     def search_for_terminating_token(
00:52:32.840292 line       179         match = program.search(tail)
New var:....... match = <re.Match object; span=(90, 92), match='}}'>
00:52:32.841333 line       180         if not match:
00:52:32.842425 line       186         start, end = match.span(1)
New var:....... start = 90
New var:....... end = 92
00:52:32.843606 line       188         nesting_match = nesting_program.match(tail, start)
New var:....... nesting_match = None
00:52:32.844716 line       189         if nesting_match:
00:52:32.846138 line       206             return pos + end
00:52:32.847240 return     206             return pos + end
Return value:.. 94

The correct start should be 94, and the correct end should be 96

I can reproduce on main, only with a line length of 95 or greater, which is very strange.

On lengths < 95, the source_string doesn't have }} in it, which prevents this issue from occurring

\n    config(\n        lf_tags_config={\n            "enabled": true,\n            "tags": {"domain": "foo", "sensitivity": "public"},\n        }\n    )\n}}\n

It might be possible to solve this with some kind of parsing logic that checks the tail to see if it contains a dictionary. Otherwise, maybe some kind of initial pass to add trailing commas to } delimiters? That would prevent the end of the dictionary from being read as a jinja comment, since it becomes ...public"},} instead of ...public"}}

benjamin-awd · 2023-10-20T17:38:55Z

Raised a proper PR on this https://github.com/tconbeer/sqlfmt/pull/508/files -- it's a bit rough around the edges but hopefully is a step forwards.

tconbeer · 2023-10-20T18:41:03Z

Thanks so much! I've been traveling - it's on my list to review this and get the other one merged today; might slip to early next week. Really appreciate your engagement here!

tconbeer closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2023

tconbeer reopened this Oct 15, 2023

tconbeer added the bug Something isn't working label Oct 15, 2023

benjamin-awd added a commit to benjamin-awd/sqlfmt that referenced this issue Oct 19, 2023

fix tconbeer#471: handle nested dictionary in jinja expression

e4007ff

benjamin-awd added a commit to benjamin-awd/sqlfmt that referenced this issue Oct 19, 2023

fix tconbeer#471: handle nested dictionary in jinja expression

94f093d

benjamin-awd added a commit to benjamin-awd/sqlfmt that referenced this issue Oct 20, 2023

fix tconbeer#471: handle nested dictionary in jinja expression

fe5a4cd

benjamin-awd added a commit to benjamin-awd/sqlfmt that referenced this issue Oct 20, 2023

fix tconbeer#471: handle nested dictionary in jinja expression

14a3b13

tconbeer pushed a commit to benjamin-awd/sqlfmt that referenced this issue Oct 20, 2023

fix tconbeer#471: handle nested dictionary in jinja expression

6d43942

tconbeer closed this as completed in 591884a Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested dict argument to jinja macro fails the formatter #471

Nested dict argument to jinja macro fails the formatter #471

rparvathaneni-sc commented Aug 31, 2023

tconbeer commented Aug 31, 2023 •

edited

Loading

benjamin-awd commented Oct 12, 2023

tconbeer commented Oct 12, 2023

benjamin-awd commented Oct 13, 2023 •

edited

Loading

tconbeer commented Oct 13, 2023

benjamin-awd commented Oct 14, 2023 •

edited

Loading

tconbeer commented Oct 15, 2023

benjamin-awd commented Oct 17, 2023

benjamin-awd commented Oct 20, 2023

tconbeer commented Oct 20, 2023

Nested dict argument to jinja macro fails the formatter #471

Nested dict argument to jinja macro fails the formatter #471

Comments

rparvathaneni-sc commented Aug 31, 2023

tconbeer commented Aug 31, 2023 • edited Loading

benjamin-awd commented Oct 12, 2023

tconbeer commented Oct 12, 2023

benjamin-awd commented Oct 13, 2023 • edited Loading

tconbeer commented Oct 13, 2023

benjamin-awd commented Oct 14, 2023 • edited Loading

tconbeer commented Oct 15, 2023

benjamin-awd commented Oct 17, 2023

benjamin-awd commented Oct 20, 2023

tconbeer commented Oct 20, 2023

tconbeer commented Aug 31, 2023 •

edited

Loading

benjamin-awd commented Oct 13, 2023 •

edited

Loading

benjamin-awd commented Oct 14, 2023 •

edited

Loading