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

Develop better rule for stubborn-merging p1 operators #214

Closed
tconbeer opened this issue Jul 19, 2022 · 0 comments · Fixed by #246
Closed

Develop better rule for stubborn-merging p1 operators #214

tconbeer opened this issue Jul 19, 2022 · 0 comments · Fixed by #246
Labels
enhancement New feature or request
Milestone

Comments

@tconbeer
Copy link
Owner

Describe the bug
Today we stubborn (or partially) merge a segment that starts with a p1 operator onto a prior segment according to this rule:

sqlfmt/src/sqlfmt/merger.py

Lines 316 to 325 in 8ee52b1

# stubbornly merge p1 operators only if they do NOT
# follow another p1 operator AND they open brackets
# and cover multiple lines
or (
not prev_operator
and self._segment_continues_operator_sequence(
segment, min_priority=1
)
and self._tail_closes_head(segment)
)

But there are edge cases where that is bad, like:
https://github.com/tconbeer/rittman_ra_data_warehouse/blob/e65dd4a8020c912a98cc87fb722055ea7c72be79/models/sources/stg_hubspot_crm/bigquery/stitch/stg_hubspot_crm_deals.sql#L233-L241

Part of the problem is that we compute prev_operator after the previous segment may have had some merging.

But the other part of the problem is that the "covers multiple lines" test doesn't try to merge the current segment first, so we end up with things like:
https://github.com/tconbeer/rittman_ra_data_warehouse/blob/e65dd4a8020c912a98cc87fb722055ea7c72be79/models/warehouse/w_marketing/wh_attribution_fact.sql#L117-L118

A better rule would be more consistent and predictable. It may rely on changing the subbornly_merge implementation to try different merging tactics with different operators (#211)

@tconbeer tconbeer added the enhancement New feature or request label Jul 19, 2022
@tconbeer tconbeer added this to the v0.11.0 milestone Aug 2, 2022
tconbeer added a commit that referenced this issue Aug 21, 2022
* refactor: remove AS and TIGHT_WORD_OPERATOR token types

* fix #214: improve rules for stubborn merging p1 operators

* refactor: improve performance regression in stubborn merging

* fix: stubbon merge p0 operators first

* fix: prevent bad stubborn merging of already-merged segments

* fix: fix regression when jinja blocks and brackets are interlaced

* chore: bump primer refs

* fix: update http primer stats
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant