New Rule ST11: Detect unused tables in join #5266

danparizher · 2023-10-02T23:27:34Z

Based on the original bones of this PR by @danparizher, I've fleshed out the logic so that it's a working rule. Fixes #5251.

NOTE: I've decided not to enable a fix routine for this rule, because it's effectively ambiguous what the intent was - and just removing a join could change the granularity of a table if not done with care. I've also decided that this rule shouldn't return unless all references are qualified - meaning it's effectively dependent on RF02 to enforce that first.

Very open to feedback!

github-actions · 2024-10-17T08:42:31Z

Coverage Results ✅

Name    Stmts   Miss  Cover   Missing
-------------------------------------
TOTAL   19084      0   100%

247 files skipped due to complete coverage.

keraion

I am a fan of this rule. I know I've run into queries that have had those unneeded joins.

My concern is it might trigger a bit too often which might lead to people disabling it entirely.

-- getting a count of two joined tables triggers this rule.
SELECT COUNT(*) AS row_count -- could also be COUNT(1) or COUNT(0)
FROM taba AS a
INNER JOIN tabb AS b
    ON a.col1 = b.col1;

-- ANTI and SEMI joins will always trigger this as well, `b` cannot be referenced
SELECT a.col1, a.col2
FROM taba AS a
SEMI JOIN tabb AS b
    ON a.col1 = b.col1;

SELECT a.col1, a.col2
FROM taba AS a
ANTI JOIN tabb AS b
    ON a.col1 = b.col1;

-- an INNER JOIN might be used like an EXISTS/SEMI JOIN
SELECT
    a.key_column,
    a.col2,
    a.col3,
    a.col4
FROM all_records AS a
INNER JOIN targeted_records AS t
    ON a.key_column = t.key_column;

It might make more sense to only apply this to LEFT, RIGHT, and FULL joins and have an exception for joins that utilize COUNT.

Overall, I'm excited to see this rule in place in the near future!

alanmcruickshank · 2024-10-21T12:42:36Z

@keraion - great feedback 👍 . On the Count(*) example and with explicit ANTI and SEMI joins I totally agree. I'll include some of those in the test cases.

On the "inner join as filter" case I totally see your point, but I also think those are some of the cases which we're probably trying to detect too 🤔 . I think you're right to suggest we exclude them, which would also exclude an unqualified JOIN too, because I think most database engines default to an INNER join if one isn't specified. In my mind, ideally we would flag this as an issue because they may often be mistakes, but do you have a way we'd recommend to people to indicate when it's intentional (rather than just suggesting a noqa line, which is my current best suggestion)?

barrywhart · 2024-10-24T12:42:03Z

src/sqlfluff/rules/structure/ST10.py

+    designed to work alongside :sqlfluff:ref:`references.qualification`
+    (:sqlfluff:ref:`RF02`).
+
+    This rule does not propose a fix, because it assumes that it an unused


Makes sense. I could imagine unusual cases where someone joins with a table simply for filtering purposes, and there's nothing "wrong" with that. (Although one could argue in that case WHERE EXISTS with a sub-select would be clearer about the intent.

To address this issue, I've added an allowlist of specific keywords, which includes INNER to prevent this situation. By making it configurable, people can make it more or less aggressive as they prefer.

barrywhart · 2024-10-24T12:44:08Z

src/sqlfluff/rules/structure/ST10.py

+    In the (*very rare*) situations that it is logically necessary to include
+    a table in a join clause, but not otherwise refer to it (likely for
+    granularity reasons, or as a stepping stone to another table), we recommend
+    ignoring this rule for that specific line by using ``-- noqa: ST10`` at


Think it's worth mentioning WHERE EXISTS as another possible remedy?

test/fixtures/rules/std_rule_cases/ST10.yml

barrywhart · 2024-10-24T12:51:23Z

test/fixtures/rules/std_rule_cases/ST10.yml

@@ -0,0 +1,65 @@
+rule: ST10


Have you considered the possibility that an "unused" table is actually referenced in a subquery? Below is a quick example of what I mean. I don't have a real-world situation in mind -- just threw together this query as an example.

SELECT a.col1 FROM a JOIN b ON a.id = b.a_id WHERE a.some_column IN ( SELECT c.some_column FROM c WHERE c.other_column = b.col )

@barrywhart - I've added your query (slightly adjusted) as a test case, and we're all good 👍 . Introspection into subqueries is covered.

src/sqlfluff/rules/structure/ST10.py

barrywhart · 2024-10-24T12:57:24Z

Could one or more of the maintainers run this rule on a large set of real-world queries? I do like the rule, but we may see surprise warnings from it for cases we hadn't considered.

alanmcruickshank · 2024-12-03T15:53:47Z

Could one or more of the maintainers run this rule on a large set of real-world queries? I do like the rule, but we may see surprise warnings from it for cases we hadn't considered.

I've run this on my current large project, and got a few flags from INNER joins being used as filters - but since adding the allowlist config - not issues. Seems to be running ok 👍

alanmcruickshank · 2024-12-03T16:55:15Z

Update post maintainers call: I've removed the proposed config value, and now it only flags on explicit LEFT, RIGHT and FULL joins. I think this is otherwise ready for a review. @WittierDinosaur ?

barrywhart · 2024-12-06T13:31:12Z

src/sqlfluff/rules/structure/ST11.py

+        mismatches.
+
+        NOTE: If *any* references aren't appropriately qualified,
+        this rule will abort (because it won't know how to resolve


Probably a nitpick, but assuming:

At least some of the references are qualified

The ones that are qualified reference all the tables

then should it be okay if some of the references are unqualified?

well... if all the tables are referenced by some of the columns, then there's no error - so this rule wouldn't fire anyway.

You're correct that we could check if not all the columns are qualified, but if we'd still only kick up an error if we were sure that a table was unused then we need both a table to be unreferenced and all columns to be qualified. Given that second condition is the easiest to check, it seems smart to do that first.

barrywhart · 2024-12-06T13:32:20Z

src/sqlfluff/rules/structure/__init__.py

@@ -21,6 +21,17 @@ def get_configs_info() -> Dict[str, Any]:
                "Defaults to ``earlier``."
            ),
        },
+        "allowlist_join_keywords": {


Is this used anywhere? I don't see any other references to allowlist_join_keywords in the PR. It looks like it's hardcoded above to {"FULL", "LEFT", "RIGHT"}.

GOOD CATCH. Yeah I can remove that 👍

barrywhart · 2024-12-06T13:33:41Z

test/fixtures/rules/std_rule_cases/ST11.yml

+    )
+
+test_fail_subquery:
+  # "b" is not referenced outside it's join (despite subquery), so unused.


it's -> its

alanmcruickshank

Thanks @barrywhart for the review. I'll get this merged now those parts are addressed 👍

Create basic structure

200ef6c

danparizher changed the title ~~Add new rule ST10: Remove unused tables in join~~ [WIP] Add new rule ST10: Remove unused tables in join Oct 2, 2023

alanmcruickshank and others added 11 commits October 7, 2023 15:12

Merge branch 'main' into main

f77fd3a

Merge branch 'main' into main

0e0ff78

Merge remote-tracking branch 'origin/main'

7a934f1

Update the docstring

1aed739

Main logic of the rule

52fec56

linting

9dc8d2f

yamllint

a2f17d5

Exception for single table selects

e8a5d66

resolve ST10

e193b07

More test cases

7cc4639

fix bug and update yaml tests

12cace4

alanmcruickshank changed the title ~~[WIP] Add new rule ST10: Remove unused tables in join~~ New rule ST10: Detect unused tables in join Oct 17, 2024

update the failing dbt test as a result

29d71af

alanmcruickshank marked this pull request as ready for review October 17, 2024 08:37

alanmcruickshank requested a review from barrywhart October 17, 2024 08:39

alanmcruickshank added the minor release A PR or issue queued for minor release. label Oct 17, 2024

alanmcruickshank changed the title ~~New rule ST10: Detect unused tables in join~~ New Rule ST10: Detect unused tables in join Oct 17, 2024

Merge branch 'main' into main

016c0c9

alanmcruickshank requested a review from keraion October 20, 2024 19:42

keraion reviewed Oct 21, 2024

View reviewed changes

barrywhart reviewed Oct 24, 2024

View reviewed changes

src/sqlfluff/rules/structure/ST10.py Outdated Show resolved Hide resolved

ST10 -> ST11

ea5b215

alanmcruickshank changed the title ~~New Rule ST10: Detect unused tables in join~~ New Rule ST11: Detect unused tables in join Dec 3, 2024

duplicate word

c55a511

alanmcruickshank and others added 2 commits December 3, 2024 15:49

More tests and configure allowlist of keywords.

b407ac8

Merge branch 'main' into main

126214d

alanmcruickshank and others added 3 commits December 3, 2024 16:48

Roll back on config changes - only flag on left right & full

4585a69

Merge branch 'main' of https://github.com/danparizher/sqlfluff

af61a90

Merge branch 'main' into main

5c91896

alanmcruickshank requested a review from WittierDinosaur December 3, 2024 16:55

alanmcruickshank added 3 commits December 5, 2024 11:42

linting

dfbf96b

Subquery test case.

5e9594e

Add a failing subquery case too

92efcca

alanmcruickshank requested a review from barrywhart December 5, 2024 12:12

alanmcruickshank mentioned this pull request Dec 5, 2024

Prep version 3.3.0 #6475

Merged

Merge branch 'main' into main

9af99cd

barrywhart reviewed Dec 6, 2024

View reviewed changes

PR Feedback

f95af9e

alanmcruickshank approved these changes Dec 6, 2024

View reviewed changes

Merge branch 'main' into main

05ecafb

alanmcruickshank enabled auto-merge December 6, 2024 16:02

alanmcruickshank added this pull request to the merge queue Dec 6, 2024

Merged via the queue into sqlfluff:main with commit 25f1248 Dec 6, 2024
29 checks passed

Uh oh!

New Rule ST11: Detect unused tables in join #5266

New Rule ST11: Detect unused tables in join #5266

Uh oh!

Conversation

danparizher commented Oct 2, 2023 • edited by alanmcruickshank Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Results ✅

Uh oh!

keraion left a comment

Choose a reason for hiding this comment

Uh oh!

alanmcruickshank commented Oct 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

barrywhart commented Oct 24, 2024

Uh oh!

alanmcruickshank commented Dec 3, 2024

Uh oh!

alanmcruickshank commented Dec 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanmcruickshank left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danparizher commented Oct 2, 2023 •

edited by alanmcruickshank

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading