L042: Enable autofix for some tricky cases #3700

barrywhart · 2022-07-31T18:22:12Z

Brief summary of the change made

Rewrite of L042 to fix a "linter looping" bug. Fixed some other previously unnoticed issues as well. (See L042.yml for details.)
The rewrite uses SelectCrawler and is a bit shorter/simpler as a result.
The rewrite surfaced and fixes some issues with SelectCrawler, especially involving nested queries and consistency around handling of subqueries in FROM clauses. Adds automated tests for SelectCrawler, which previously had none.

Fixes #3598

Are there any other side effects of this change that we should be aware of?

The changes to SelectCrawler could surface some issues (e.g. I had to update L028 to get a test to pass).

Pull Request checklist

Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
- .yml rule test cases in test/fixtures/rules/std_rule_cases.
- .sql/.yml parser test cases in test/fixtures/dialects (note YML files can be auto generated with tox -e generate-fixture-yml).
- Full autofix test cases in test/fixtures/linter/autofix.
- Other.
Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.

codecov · 2022-07-31T18:35:58Z

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (cf24595) compared to base (1000cf1).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #3700   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          179       179           
  Lines        13729     13752   +23     
=========================================
+ Hits         13729     13752   +23

Impacted Files	Coverage Δ
src/sqlfluff/core/parser/segments/base.py	`100.00% <100.00%> (ø)`
src/sqlfluff/rules/L028.py	`100.00% <100.00%> (ø)`
src/sqlfluff/rules/L042.py	`100.00% <100.00%> (ø)`
src/sqlfluff/utils/analysis/select_crawler.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

alanmcruickshank · 2022-08-15T08:54:30Z

@barrywhart - are you ok incorporating #3717 into this or do you need a hand?

barrywhart · 2022-08-15T12:04:17Z

@alanmcruickshank: I'm okay, thanks! This is a slow-moving, slightly experimental PR anyway.

Failures: FAILED test/rules/yaml_test_cases_test.py::test__rule_test_case[L042_with_fail] FAILED test/rules/yaml_test_cases_test.py::test__rule_test_case[L042_issue_3572_correlated_subquery_3] FAILED test/rules/yaml_test_cases_test.py::test__rule_test_case[L042_issue_3598_avoid_looping_2] - AssertionError: assert 'WITH prep_1 ...* FROM cte1\n' == 'WITH temp AS...* FROM cte1\n'

barrywhart · 2022-09-12T21:48:09Z

test/fixtures/rules/std_rule_cases/L042.yml

+        FROM (SELECT a)
+    )
+    SELECT a FROM cte1
+  fix_str: |


Fixes previously unfixable issue that had previously triggered linter looping

barrywhart · 2022-09-13T11:01:20Z

src/sqlfluff/rules/L028.py

+        children = list(query.children)
+        # 'query.children' includes CTEs and "main" queries, but not queries in
+        # the "FROM" list. We want to visit those as well.
+        for a in select_info.table_aliases if select_info else []:


This additional code is now required because of the SelectCrawler improvements. Previously, the behavior of SelectCrawler was inconsistent about whether it returned queries in the FROM list.

…thub.com/barrywhart/sqlfluff into bhart-issue_3598_l042_fix_tricky_queries

barrywhart · 2022-09-13T11:27:32Z

test/fixtures/rules/std_rule_cases/L042.yml

+  violations_after_fix:
+    - description: select_statement clauses should not contain subqueries. Use CTEs instead
+      line_no: 2
+      line_pos: 20


violations_after_fix because the two as b aliases collide, preventing autofix from fixing the second one.

OTooleMichael

LGTM - mostly style comments

OTooleMichael · 2022-09-13T13:11:20Z

src/sqlfluff/rules/L028.py

+        children = list(query.children)
+        # 'query.children' includes CTEs and "main" queries, but not queries in
+        # the "FROM" list. We want to visit those as well.
+        for a in select_info.table_aliases if select_info else []:


Suggested change

for a in select_info.table_aliases if select_info else []:

for a in (select_info.table_aliases if select_info else []):

perhaps brackets or a variable, the turnary in line with a loop is an overload for me

I'll change it. Probably will add an if around it.

OTooleMichael · 2022-09-13T13:12:16Z

src/sqlfluff/rules/L028.py

+                if isinstance(q, Query):
+                    # Check for previously visited selectables to avoid possible


Suggested change

if isinstance(q, Query):

# Check for previously visited selectables to avoid possible

if not isinstance(q, Query):

continue

# Check for previously visited selectables to avoid possible

OTooleMichael · 2022-09-13T13:12:46Z

src/sqlfluff/rules/L028.py

+                    if not any(s.selectable in visited for s in q.selectables):
+                        visited.update(s.selectable for s in q.selectables)


Suggested change

if not any(s.selectable in visited for s in q.selectables):

visited.update(s.selectable for s in q.selectables)

if any(s.selectable in visited for s in q.selectables):

continue

visited.update(s.selectable for s in q.selectables)

OTooleMichael · 2022-09-13T13:22:50Z

src/sqlfluff/rules/L042.py

+        """Given the root query, compute lint warnings."""
+        parent_types = self._config_mapping[self.forbid_subquery_in]
+        for q in [query] + list(query.ctes.values()):
+            for selectable in q.selectables:
+                if not selectable.select_info:
+                    continue


This method feels quite dense, I can't really make my way through it. Is there any code splitting possible?

Probably. I'll take a look.

L042: Enable autofix for some tricky cases

81d3061

barrywhart marked this pull request as draft July 31, 2022 18:22

barrywhart added 6 commits July 31, 2022 17:44

Fixes "SELECT a" query!

c4a777a

Update the two "avoid looping" tests now that they get fixed

2932e72

Preserve brackets, whitespace around extracted CTE

d9109d9

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

0bc8393

Add automated test for SelectCrawler, fix an old bug I just discovered

23aafe8

Don't try and create a CTE from an existing CTE

760108b

barrywhart added 19 commits August 28, 2022 12:43

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

d93ab53

Fix broken test

b9db4ac

Fix some tests, issues involving aliased subselects in JOIN clauses

3eef220

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

6a2cc12

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

f60cb58

Implement 'forbid_subquery_in' config

f07ae2b

Update test case: Seems fixable now?

192db3e

Look in existing CTEs

a940c61

Detect and ignore correlated subqueries

39ee43d

Move variable definition (potentially undefined variable)

158beb8

Add violations_after_fix

c7ae515

Add fix_str and violations_after_fix

cef7ac5

Move test file to appropriate directory

698c6b3

Fix SelectCrawler inconsistency ignoring from_expression_segment

761dfe7

Fix SelectCrawler bug handling nested CTEs

1572719

Fix "Rules critical errors" failure

fa57a1f

Fix broken test

6492bd6

Fix broken test

030bb49

barrywhart added 2 commits September 12, 2022 17:34

Tidying

5af64be

Tidying

7bb3bbc

barrywhart marked this pull request as ready for review September 12, 2022 21:42

barrywhart requested review from OTooleMichael, alanmcruickshank and tunetheweb September 12, 2022 21:44

barrywhart commented Sep 12, 2022

View reviewed changes

barrywhart added 2 commits September 13, 2022 06:15

Tidying

b76a6eb

Fix L028 issue introduced by SelectCrawler changes

08dbd5d

barrywhart commented Sep 13, 2022

View reviewed changes

barrywhart added 4 commits September 13, 2022 07:10

Tidying

626e057

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

4318eb3

Remove type annotation that was triggering other mypy errors

392fca6

Merge branch 'bhart-issue_3598_l042_fix_tricky_queries' of https://gi…

bd01c1c

…thub.com/barrywhart/sqlfluff into bhart-issue_3598_l042_fix_tricky_queries

barrywhart commented Sep 13, 2022

View reviewed changes

Fix infinite recursion issue

6f536f9

OTooleMichael approved these changes Sep 13, 2022

View reviewed changes

barrywhart added 2 commits September 13, 2022 15:50

Merge branch 'main' into bhart-issue_3598_l042_fix_tricky_queries

c1c5d39

PR review

cf24595

barrywhart merged commit bca4bfc into sqlfluff:main Sep 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

L042: Enable autofix for some tricky cases #3700

L042: Enable autofix for some tricky cases #3700

barrywhart commented Jul 31, 2022 •

edited

codecov bot commented Jul 31, 2022 •

edited

alanmcruickshank commented Aug 15, 2022

barrywhart commented Aug 15, 2022

barrywhart Sep 12, 2022 •

edited

barrywhart Sep 13, 2022

barrywhart Sep 13, 2022

OTooleMichael left a comment

OTooleMichael Sep 13, 2022

barrywhart Sep 13, 2022

OTooleMichael Sep 13, 2022

barrywhart Sep 13, 2022

OTooleMichael Sep 13, 2022

barrywhart Sep 13, 2022

OTooleMichael Sep 13, 2022

barrywhart Sep 13, 2022

	for a in select_info.table_aliases if select_info else []:
	for a in (select_info.table_aliases if select_info else []):

		if isinstance(q, Query):
		# Check for previously visited selectables to avoid possible

		if not any(s.selectable in visited for s in q.selectables):
		visited.update(s.selectable for s in q.selectables)

L042: Enable autofix for some tricky cases #3700

L042: Enable autofix for some tricky cases #3700

Conversation

barrywhart commented Jul 31, 2022 • edited

Brief summary of the change made

Are there any other side effects of this change that we should be aware of?

Pull Request checklist

codecov bot commented Jul 31, 2022 • edited

Codecov Report

alanmcruickshank commented Aug 15, 2022

barrywhart commented Aug 15, 2022

barrywhart Sep 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OTooleMichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barrywhart commented Jul 31, 2022 •

edited

codecov bot commented Jul 31, 2022 •

edited

barrywhart Sep 12, 2022 •

edited