Repository: sqlfluff/sqlfluff
Pull request: L042: #89 No Subqueries in Joins
This resolves #89 (and also the oldest open issue right now!)

This flags up any subqueries in join clauses, indicating that the user should use CTEs instead.

Small note, I also corrected the naming of `MainTableExpressionGrammar` to `MainTableExpressionSegment` because it was a `Segment` and not a `Grammar`. 😄 

Problem statement:
Rule: No nested selects within join clauses.
This is a pretty general, high-level quality check. I often see this issue with SQL written by new data scientists or engineers -- they'll deliver a working query, but it's monolithic and 50+ lines long. Even with proper indentation, it's hard to grasp what going on.

When this occurs, SQLFluff should suggest breaking the query into two or more logical chunks, with some of these chunks written as (descriptively named) CTEs.

Hints from issue discussion:
I like this as an idea. Do you think it's about nested `SELECT` statements here? That would be the easiest to put a rule around: i.e. More than 3 nested select statements raises a linting error.

What I'm not sure about is whether that catches all the cases that you're thinking about. @barrywhart  Anything else you think we might need to do?
I'll review some our earlier queries and get back with you. My main memory is the number of physical lines. I don't recall whether it was mostly nesting or something else.

This reminds me of another thing -- I think some queries were JOINing with a nested SELECT for no reason -- the nested query was trivial and they could've simply written it as a JOIN. Once I find some of these queries, I'll share them here and we can think about whether this should be one issue or several.
This is one query I inherited. It's hard to point to any one single area where it is overly complex. It's just... large. 🙂 

```
SELECT
users_info.user_id AS user_id,
users_info.member_since AS member_since, 
orders_mth.paid_time AS order_date,
orders_mth.amount AS amount,
users_info.country AS country
FROM 
(SELECT ui.user_id as user_id, 
cohort.first_order_date as first_order_date,
ui.member_since AS member_since,
MAX(uv.name) AS country
FROM
(select user_id, user_type , min(date(member_since)) as member_since
from users_info
group by 1,2
having user_type != 'internal'
  ) as ui
LEFT JOIN users_contact uc
ON ui.user_id = uc.user_id
LEFT JOIN countries uv
ON uc.country=uv.country_id
left join 
(select user_id, min(date(time_stamp)) as first_order_date 
from orders
where (internal = 'N' and successful = 'Y' and monthly_order_type != 0) 
group by 1) as cohort 
ON (ui.user_id = cohort.user_id)
WHERE ( ui.member_since <= '", cohort_end_date, "' 
and ui.member_since >='", cohort_start_date, "'
and ui.user_id not in (72410650, 72458194, 72418426, 72447882)
and NOT REGEXP_CONTAINS(LOWER(uc.email), r'@mailchimp.com')
and NOT REGEXP_CONTAINS(LOWER(uc.email), r'@rsglab.com')
AND ui.user_type != 'internal'
AND cohort.first_order_date is not null 
and uc.email is not null)
GROUP BY 1,2,3) AS users_info
left join 
(select tl.user_id as user_id, date(tl.paid_time) as paid_time,
sum(tl.amount) - sum(coalesce(tl_ad.ad_amount, 0)) as amount
from  
(select time_stamp as paid_time, 
user_id as user_id,
sum(case when tax_amount > 0 and amount > 0 and amount > tax_amount
and amount = coalesce(pre_tax_amount, 0) + coalesce(tax_amount, 0) - coalesce(credits_used, 0)
then amount - tax_amount
else amount end) as amount
from orders
where (internal = 'N' and successful = 'Y' 
and monthly_order_type != 0 )
group by 1, 2) as tl
left join 
(select o.user_id as user_id, time_stamp as paid_time, 
sum(oi.discounted_amount) as ad_amount
from orders`as o
inner join order_items as oi
on (oi.order_id = o.order_id
and oi.source_database = o.source_database)
where (o.successful = 'Y'
and o.internal = 'N'
and oi.type in ('fb_ad','fb_ad_reup','google_ad','google_ad_reup','postcards_automation_credits','postcards_prepaid_postage',
'postcards_prepaid_printing')
and monthly_order_type != 0)
group by 1,2) as tl_ad 
on tl.user_id = tl_ad.user_id
and tl.paid_time = tl_ad.paid_time  
group by 1,2) AS orders_mth
on users_info.user_id = orders_mth.user_id
```
Query with unnecessary nested SELECTs:
```
SELECT
    *
  FROM (
    SELECT
      user_id,
      campaign_id,
      unique_opens,
      opens
    FROM
      `{{bi_user_campaigns_stats}}`
    JOIN (
      SELECT
        user_id,
        campaign_id
      FROM
        `{{bi_user_campaigns_contents}}`
      WHERE
        track_opens="Y")
    USING
      (user_id,
        campaign_id))
  JOIN (
    SELECT
      user_id,
      campaign_id,
      recipients_click,
      unsubs
    FROM
      `{{bi_user_campaigns_stats}}`
    JOIN (
      SELECT
        user_id,
        campaign_id
      FROM
        `{{bi_user_campaigns_content}}`
      WHERE
        track_clicks_html="Y")
    USING
      (user_id,
        campaign_id))
  USING
    (user_id,
      campaign_id)
  )
```

Another query with unnecessary nesting (FWIW, I think it's a later version of the previous one).

Perhaps SQLFluff could detect at least some of these cases automatically. In fact, is there *ever* a reason to `SELECT FROM` a nested `SELECT`? Offhand, I can't think so.

```
SELECT
  user_id,
  campaign_id,
  start_time,
  end_time,
  emails_sent,
  unique_opens,
  opens,
  recipients_click,
  unsubs
FROM
(SELECT
  c.user_id as user_id,
  c.campaign_id,
  c.start_time,
  c.end_time,
  c.emails_sent,
  s.unique_opens,
  s.opens,
  s.recipients_click,
  s.unsubs
FROM
  `{{bi_user_campaigns}}` AS c
LEFT JOIN `mc-business-intelligence.mailchimp.users_compliance` AS compliance
USING (user_id, campaign_id)
JOIN `{{bi_user_campaigns_stats}}` AS s
USING (user_id, campaign_id)
JOIN `{{bi_user_campaigns_content}}` AS content
ON (s.user_id = content.user_id
      AND s.campaign_id = content.campaign_id
      -- We only want to include campaigns that had tracking for clicks AND opens enabled
      AND track_opens="Y"
      AND track_clicks_html="Y")
WHERE
  c.type = 'regular'
  AND compliance.user_id IS NULL
  AND compliance.campaign_id IS NULL)
-- Note: At this time, campaign date filtering is only for use by tests. We
-- filter campaigns by start_date, which could have issues, e.g. including
-- stats for campaigns that are still ongoing.
WHERE TRUE {{user_filter}} {{campaign_date_filter}}
```
I think your suggestion for #37 is actually a great idea for *this* potential issue. I'm not yet sold on #37 because I think it can be quite useful to have logic in joins, but I'm definitely sold on this one, and using [Halstead Complexity Measures](https://en.wikipedia.org/wiki/Halstead_complexity_measures), sounds like a great idea.

I'm assuming this is just for select statements for now?
I agree, #37 is tricky. There are cases where a `JOIN` condition belongs in a `WHERE` instead, but I don't know if it's possible to automatically identify such cases.

Yes, I am just thinking of select statements.
In regards to recommending CTEs based on complexity, what about a rule to recommend a CTE for any subqueries that are not the first relation in the FROM clause.

Essentially, this is okay:

```sql
SELECT
    tbl_a.a,
    tbl_b.b
FROM (
    SELECT *
    -- ...
    -- ...
    -- ...
    -- ...
    -- ...
    -- ...
    FROM tbl_c
) AS tbl_a
LEFT JOIN tbl_b
    USING (keycol)
```

But this is not:

```
SELECT
    tbl_a.a,
    tbl_b.b
FROM tbl_a
LEFT JOIN (
    SELECT *
        -- ...
        -- ...
        -- ...
        -- ...
        -- ...
        -- ...
    FROM tbl_c
) AS tbl_b
    USING (keycol)
```

The logic is that whatever comes first in your query _should_ be the thing primarily determining its granularity. Also, putting extra complexity into a query right after FROM _does not at all inhibit the readability of anything in the JOIN clause_. However, once we begin our joins, the extra verbosity spreads out the code so that the full join clause may not be viewable in a single screen height.

@aaronsteers I like your suggestion! I think there might be other potential rules that could also be useful here, but your suggestion would be a really good starting point, 👍 
I've renamed this issue to update the intent of what we've scoped out.

This rule should flag on any `join` clauses which contain a `select` or `with.... select` statements.

I think adding a `fix` for this rule would be _possible_, but definitely not trivial so I think for now we wouldn't do that.

Commit message: L042: #89 No Subqueries in Joins

Patch to review:
diff --git a/src/sqlfluff/core/dialects/dialect_ansi.py b/src/sqlfluff/core/dialects/dialect_ansi.py
index 016383b07d5..a15793e8cb0 100644
--- a/src/sqlfluff/core/dialects/dialect_ansi.py
+++ b/src/sqlfluff/core/dialects/dialect_ansi.py
@@ -812,8 +812,8 @@ class TableExpressionSegment(BaseSegment):
     match_grammar = Sequence(
         Ref("PreTableFunctionKeywordsGrammar", optional=True),
         OneOf(
-            Ref("MainTableExpressionGrammar"),
-            Bracketed(Ref("MainTableExpressionGrammar")),
+            Ref("MainTableExpressionSegment"),
+            Bracketed(Ref("MainTableExpressionSegment")),
         ),
         Ref("AliasExpressionSegment", optional=True),
         Ref("PostTableExpressionGrammar", optional=True),
@@ -847,7 +847,7 @@ def get_eventual_alias(self) -> Optional[AliasInfo]:
 
 
 @ansi_dialect.segment()
-class MainTableExpressionGrammar(BaseSegment):
+class MainTableExpressionSegment(BaseSegment):
     """The main table expression e.g. within a FROM clause."""
 
     type = "main_table_expression"
diff --git a/src/sqlfluff/core/dialects/dialect_exasol.py b/src/sqlfluff/core/dialects/dialect_exasol.py
index a34eb1d9a41..71cdc6044f5 100644
--- a/src/sqlfluff/core/dialects/dialect_exasol.py
+++ b/src/sqlfluff/core/dialects/dialect_exasol.py
@@ -226,7 +226,7 @@ class SelectStatementSegment(BaseSegment):
 
 
 @exasol_dialect.segment(replace=True)
-class MainTableExpressionGrammar(BaseSegment):
+class MainTableExpressionSegment(BaseSegment):
     """The main table expression e.g. within a FROM clause."""
 
     type = "main_table_expression"
diff --git a/src/sqlfluff/core/rules/std/L042.py b/src/sqlfluff/core/rules/std/L042.py
new file mode 100644
index 00000000000..e8e9b5ab305
--- /dev/null
+++ b/src/sqlfluff/core/rules/std/L042.py
@@ -0,0 +1,64 @@
+"""Implementation of Rule L042."""
+
+from sqlfluff.core.rules.base import BaseCrawler, LintResult
+
+
+class Rule_L042(BaseCrawler):
+    """Join clauses should not contain subqueries. Use CTEs instead.
+
+    NB: Some dialects don't allow CTEs, and for those dialects
+    this rule makes no sense and should be disabled.
+
+    | **Anti-pattern**
+
+    .. code-block:: sql
+
+        select
+            a.x, a.y, b.z
+        from a
+        join (
+            select x, z from b
+        ) using(x)
+
+
+    | **Best practice**
+
+    .. code-block:: sql
+
+        with c as (
+            select x, z from b
+        )
+        select
+            a.x, a.y, c.z
+        from a
+        join c using(x)
+
+    """
+
+    def _eval(self, segment, **kwargs):
+        """Join clauses should not contain subqueries. Use CTEs instead.
+
+        NB: No fix for this routine because it would be very complex to
+        implement reliably.
+        """
+        if segment.is_type("join_clause"):
+            # Get the referenced table segment
+            table_expression = segment.get_child("table_expression")
+            if not table_expression:
+                return None  # There isn't one. We're done.
+            # Get the main bit
+            table_expression = table_expression.get_child("main_table_expression")
+            if not table_expression:
+                return None  # There isn't one. We're done.
+
+            # If any of the following are found, raise an issue.
+            # If not, we're fine.
+            problem_children = [
+                "with_compound_statement",
+                "set_expression",
+                "select_statement",
+            ]
+            for seg_type in problem_children:
+                seg = table_expression.get_child(seg_type)
+                if seg:
+                    return LintResult(anchor=seg)
diff --git a/test/core/rules/test_cases/L042.yml b/test/core/rules/test_cases/L042.yml
new file mode 100644
index 00000000000..4f039d6d3a9
--- /dev/null
+++ b/test/core/rules/test_cases/L042.yml
@@ -0,0 +1,43 @@
+rule: L042
+
+select_fail:
+  fail_str: |
+    select
+        a.x, a.y, b.z
+    from a
+    join (
+        select x, z from b
+    ) on (a.x = b.x)
+  
+with_fail:
+  fail_str: |
+    select
+        a.x, a.y, b.z
+    from a
+    join (
+        with d as (
+            select x, z from b
+        )
+        select * from d
+    ) using (x)
+  
+set_fail:
+  fail_str: |
+    select
+        a.x, a.y, b.z
+    from a
+    join (
+        select x, z from b
+        union
+        select x, z from d
+    ) using (x)
+
+simple_pass:
+  pass_str: |
+    with c as (
+        select x, z from b
+    )
+    select
+        a.x, a.y, c.z
+    from a
+    join c on (a.x = c.x)


Code review comments:
diff_hunk: @@ -0,0 +1,64 @@
+"""Implementation of Rule L042."""
+
+from sqlfluff.core.rules.base import BaseCrawler, LintResult
+
+
+class Rule_L042(BaseCrawler):
+    """Join clauses should not contain subqueries. Use CTEs instead.
+
+    NB: Some dialects don't allow CTEs, and for those dialects
+    this rule makes no sense and should be disabled.
+
+    | **Anti-pattern**
+
+    .. code-block:: sql
+
+        select
+            a.x, a.y, b.z
+        from a
+        join (
+            select x, z from b
+        ) using(x)
+
+
+    | **Best practice**
+
+    .. code-block:: sql
+
+        with c as (
+            select x, z from b
+        )
+        select
+            a.x, a.y, c.z
+        from a
+        join c using(x)
+
+    """
+
+    def _eval(self, segment, **kwargs):
+        """Join clauses should not contain subqueries. Use CTEs instead.
+
+        NB: No fix for this routine because it would be very complex to
+        implement reliably.
+        """
+        if segment.is_type("join_clause"):
+            # Get the referenced table segment
+            table_expression = segment.get_child("table_expression")
path: src/sqlfluff/core/rules/std/L042.py
text: @user1:
Does this handle the case of multiple joins, e.g.:
```
SELECT c
FROM t1
JOIN t2
ON <<something>>
JOIN (
    SELECT x, z FROM t3
)
```

It may very well work, mainly I'm unsure how the grammar is structured. Worth adding a test for this?

@author:
It should cover it yes, but I'll add a test case to confirm that.

diff_hunk: @@ -0,0 +1,64 @@
+"""Implementation of Rule L042."""
+
+from sqlfluff.core.rules.base import BaseCrawler, LintResult
+
+
+class Rule_L042(BaseCrawler):
+    """Join clauses should not contain subqueries. Use CTEs instead.
+
+    NB: Some dialects don't allow CTEs, and for those dialects
path: src/sqlfluff/core/rules/std/L042.py
text: @user1:
I feel like that should be an issue?

I think the base rule class "BaseCrawler" should have an `exclude_dialects = ` optional property

@author:
great idea - but in this case it wouldn't help. It's only older versions of mysql which don't support it, newer ones are fine. Do you know of any other rules which might use a feature like this?