Repository: tobymao/sqlglot
Pull request: Fix!(optimizer): remove redundant parens during subquery elimination
Fixes #2075

Problem statement:
Optimizer is incorrect when superfluous parens wrap subqueries
```
from sqlglot import parse_one
from sqlglot.optimizer import optimize
from sqlglot.dialects import Snowflake

sql = """
SELECT 
  ("SUBQUERY_0"."KEY") AS "SUBQUERY_1_COL_0"
FROM 
  (
    SELECT 
      * 
    FROM 
      ((( -- <-------------- ***note multiple parens****
          select 
            * 
          from 
            (
              select 
                event_name as key, 
                insert_ts 
              from 
                (
                  select 
                    insert_ts, 
                    event_name 
                  from 
                    sales
                  where 
                    insert_ts > '2023-08-07 21:03:35.590 -0700'
                )
        )
      ))) AS "SF_CONNECTOR_QUERY_ALIAS"
  ) AS "SUBQUERY_0"

"""

print("========= with parens ==========")
optimized = optimize(parse_one(sql, dialect=Snowflake), dialect=Snowflake)
print(optimized.sql(pretty=True))

print("========= without parens ==========")
without_double_parens = sql.replace("((", "").replace("))","")
optimized = optimize(parse_one(without_double_parens, dialect=Snowflake), dialect=Snowflake)
print(optimized.sql(pretty=True))
```
returns

```
========= with parens ==========
SELECT
  "SF_CONNECTOR_QUERY_ALIAS"."KEY" AS "SUBQUERY_1_COL_0"
FROM (
  (
    "cte" AS "cte"
  )
) AS "SF_CONNECTOR_QUERY_ALIAS"
========= without parens ==========
SELECT
  "SALES"."EVENT_NAME" AS "SUBQUERY_1_COL_0"
FROM "SALES" AS "SALES"
WHERE
  "SALES"."INSERT_TS" > '2023-08-07 21:03:35.590 -0700'
```

I've run into a case where the optimizer is sensitive to the number of [superfluous] parentheses wrapping a subquery, and further returns incorrect logic in the "with parens" case. Note entirely missing `where` clause in the `with parens` case.

The parsed ASTs seem to be the same, so I'm assuming this is in the optimizer somewhere.

Hints from issue discussion:

Commit message: Fix!(optimizer): remove redundant parens during subquery elimination

Patch to review:
diff --git a/sqlglot/optimizer/eliminate_subqueries.py b/sqlglot/optimizer/eliminate_subqueries.py
index af42f25aca..72bb200852 100644
--- a/sqlglot/optimizer/eliminate_subqueries.py
+++ b/sqlglot/optimizer/eliminate_subqueries.py
@@ -142,13 +142,21 @@ def _eliminate_derived_table(scope, existing_ctes, taken):
     if scope.parent.pivots or isinstance(scope.parent.expression, exp.Lateral):
         return None
 
-    parent = scope.expression.parent
+    # Get rid of redundant exp.Subquery expressions, i.e. those that are used as parentheses
+    to_replace = scope.expression.parent
+    while (
+        isinstance(to_replace, exp.Subquery)
+        and to_replace.same_parent
+        and not any(v is not None and k != "this" for k, v in to_replace.args.items())
+    ):
+        to_replace = to_replace.parent
+
     name, cte = _new_cte(scope, existing_ctes, taken)
+    table = exp.alias_(exp.table_(name), alias=to_replace.alias or name)
+    table.set("joins", to_replace.args.get("joins"))
 
-    table = exp.alias_(exp.table_(name), alias=parent.alias or name)
-    table.set("joins", parent.args.get("joins"))
+    to_replace.replace(table)
 
-    parent.replace(table)
     return cte
 
 
diff --git a/tests/fixtures/optimizer/eliminate_subqueries.sql b/tests/fixtures/optimizer/eliminate_subqueries.sql
index 8542c3e59d..e18d2a4590 100644
--- a/tests/fixtures/optimizer/eliminate_subqueries.sql
+++ b/tests/fixtures/optimizer/eliminate_subqueries.sql
@@ -97,3 +97,7 @@ WITH cte1 AS (SELECT a FROM x), cte2 AS (SELECT a FROM cte1 AS cte3) SELECT a FR
 -- Wrapped subquery joined with table
 SELECT * FROM ((SELECT c FROM t1) JOIN t2);
 WITH cte AS (SELECT c FROM t1) SELECT * FROM (cte AS cte, t2);
+
+-- Wrapped subquery with redundant parentheses
+SELECT * FROM (((SELECT * FROM tbl)));
+WITH cte AS (SELECT * FROM tbl) SELECT * FROM cte AS cte;
diff --git a/tests/fixtures/optimizer/optimizer.sql b/tests/fixtures/optimizer/optimizer.sql
index b318a92382..18ee8042c0 100644
--- a/tests/fixtures/optimizer/optimizer.sql
+++ b/tests/fixtures/optimizer/optimizer.sql
@@ -950,3 +950,40 @@ JOIN "n" AS "foo"("a")
 SELECT CONCAT('a', 'b') || CONCAT(CONCAT('c', 'd'), CONCAT('e', 'f')) + ('g' || 'h' || 'i');
 SELECT
   'abcdefghi' AS "_col_0";
+
+# title: complex query with derived tables and redundant parentheses
+# execute: false
+# dialect: snowflake
+SELECT
+  ("SUBQUERY_0"."KEY") AS "SUBQUERY_1_COL_0"
+FROM
+  (
+    SELECT
+      *
+    FROM
+      (((
+          SELECT
+            *
+          FROM
+            (
+              SELECT
+                event_name AS key,
+                insert_ts
+              FROM
+                (
+                  SELECT
+                    insert_ts,
+                    event_name
+                  FROM
+                    sales
+                  WHERE
+                    insert_ts > '2023-08-07 21:03:35.590 -0700'
+                )
+            )
+      ))) AS "SF_CONNECTOR_QUERY_ALIAS"
+  ) AS "SUBQUERY_0";
+SELECT
+  "SALES"."EVENT_NAME" AS "SUBQUERY_1_COL_0"
+FROM "SALES" AS "SALES"
+WHERE
+  "SALES"."INSERT_TS" > '2023-08-07 21:03:35.590 -0700';


Code review comments:
diff_hunk: @@ -142,13 +142,21 @@ def _eliminate_derived_table(scope, existing_ctes, taken):
     if scope.parent.pivots or isinstance(scope.parent.expression, exp.Lateral):
         return None
 
-    parent = scope.expression.parent
+    # Get rid of redundant exp.Subquery expressions, i.e. those that are used as parentheses
+    to_replace = scope.expression.parent
+    while (
+        isinstance(to_replace, exp.Subquery)
+        and to_replace.same_parent
+        and not any(v is not None and k != "this" for k, v in to_replace.args.items())
+    ):
+        to_replace = to_replace.parent
path: sqlglot/optimizer/eliminate_subqueries.py
text: @author:
The idea here is that we don't want remove subqueries that have additional args, e.g. laterals, alias, joins, ...

Perhaps we can also aggregate these "special" args into a set, so that we can avoid checking _all_ query modifier args in `exp.Subquery` and just focus on them? This seemed more safer for the time being, but happy to investigate more.

@user1:
can this just be implement in def unnest?

@author:
Where do you mean?

@user1:
there's a method unnest that is supposed to do this behavior

@author:
I'll take a look, maybe we can do this "unnesting" at an earlier stage like you say, although `Subquery.unnest` simply returns the first non-`Subquery` so I'm not sure if it's very relevant. This seems like more specialized logic to me, mostly related with how scoping works.

@author:
@user1 I took a stab at this, can you take another look? Happy to pivot if this is not what you had in mind.