Repository: tobymao/sqlglot
Pull request: fix(redshift): Add unsupported warnings for UNNEST
Fixes #4169

Redshift supports [implicit unnest of an array](https://stackoverflow.com/a/72870096) but not an `UNNEST` function as other dialects. This PR adds unsupported warnings for the following cases:
- If an `exp.Unnest` is used with a _known_ non-array type
- If an `exp.Unnest` is not used under a `FROM` or `JOIN` clause
- If we attempt to transpile an `exp.Explode` node coming from Spark/DuckDB etc.

Problem statement:
incorrect transpilation of unnest for redshift
running:
`sqlglot.transpile("select unnest({\"a\": 123}) as x;", read="duckdb", write="redshift")[0]`

actual:
`'SELECT UNNEST(STRUCT(123 AS a)) AS x'`
however redshift doesn't support unnest. there is no perfect equivalent in redshift so i'm curious if this is a situation where sqlglot should do the full surgery into multiple stages of queries or just throw its hands up.

the equivalent output to tsql does the right thing.
` sqlglot.transpile("select unnest({\"a\": 123}) as x;", read="duckdb", write="tsql")[0]`
returns:
`'SELECT EXPLODE(STRUCT(123 AS a)) AS x'`

Hints from issue discussion:
I think the highest ROI for now is to use `self.unsupported` if `UNNEST` is being used as a projection or something, @VaggelisD could you take a look?

@bjabes how complicated would it be to convert the DuckDB query into an equivalent Redshift one?
Yeah this isn't blocking anything on my side, it was an exploration for now!
if your question is "what is the equivalent of this in redshift" – i'll form the best version for you i can. it's going to be not perfectly the same.

oh and i'm not sure that struct syntax is appropriate for redshift either
ok i think there's 2 issues here. first is that json literals should probably be converted into `json_parse('<literal>')` and the second is converting unnest. i'll create the other issue unless you prefer to keep it all together here.
let's keep this here, but we may not address this because it's not super high priority for us
i nerded out on this some more. it's basically a nightmare to clone "unnest" in redshift (requires a stored proc because no dynamic sql possible). i would mark that as `unsupported` in redshift.

i _would_ fix the json -> struct to convert a json literal to `json_parse('<literal>')`
Hey @bjabes, really appreciate the analysis, I'll add the unsupported warning to `UNNEST`.

Regarding the JSON literals, do you mind providing more context and possibly some examples? From what I understood at first, in DuckDB we e.g. extract values from string literals but in Redshift they have to be wrapped in `JSON_PARSE(...)`, is that an JSON -> STRUCT operation?

Commit message: fix(redshift): Add unsupported warnings for UNNEST

Patch to review:
diff --git a/sqlglot/dialects/redshift.py b/sqlglot/dialects/redshift.py
index 77e54e3f36..66847f0f8e 100644
--- a/sqlglot/dialects/redshift.py
+++ b/sqlglot/dialects/redshift.py
@@ -184,6 +184,7 @@ class Generator(Postgres.Generator):
             exp.DateDiff: date_delta_sql("DATEDIFF"),
             exp.DistKeyProperty: lambda self, e: self.func("DISTKEY", e.this),
             exp.DistStyleProperty: lambda self, e: self.naked_property(e),
+            exp.Explode: lambda self, e: self.explode_sql(e),
             exp.FromBase: rename_func("STRTOL"),
             exp.GeneratedAsIdentityColumnConstraint: generatedasidentitycolumnconstraint_sql,
             exp.JSONExtract: json_extract_segments("JSON_EXTRACT_PATH_TEXT"),
@@ -388,13 +389,28 @@ def unnest_sql(self, expression: exp.Unnest) -> str:
             args = expression.expressions
             num_args = len(args)
 
-            if num_args > 1:
+            if not num_args == 1:
                 self.unsupported(f"Unsupported number of arguments in UNNEST: {num_args}")
                 return ""
 
-            arg = self.sql(seq_get(args, 0))
+            if not expression.find_ancestor(exp.From, exp.Join):
+                self.unsupported("Unsupported UNNEST when not used in FROM/JOIN clauses")
+                return ""
+
+            from sqlglot.optimizer.annotate_types import annotate_types
+
+            arg = t.cast(exp.Expression, seq_get(args, 0))
+            annotated_arg = annotate_types(arg)
+            if not annotated_arg.is_type(exp.DataType.Type.UNKNOWN) and not annotated_arg.is_type(
+                exp.DataType.Type.ARRAY
+            ):
+                self.unsupported("Unsupported UNNEST without ARRAY argument")
+                return ""
+
+            arg_sql = self.sql(arg)
+
             alias = self.expressions(expression.args.get("alias"), key="columns", flat=True)
-            return f"{arg} AS {alias}" if alias else arg
+            return f"{arg_sql} AS {alias}" if alias else arg_sql
 
         def cast_sql(self, expression: exp.Cast, safe_prefix: t.Optional[str] = None) -> str:
             if expression.is_type(exp.DataType.Type.JSON):
@@ -434,3 +450,7 @@ def array_sql(self, expression: exp.Array) -> str:
                 return super().array_sql(expression)
 
             return rename_func("ARRAY")(self, expression)
+
+        def explode_sql(self, expression: exp.Explode) -> str:
+            self.unsupported("Unsupported EXPLODE() function")
+            return ""
diff --git a/tests/dialects/test_duckdb.py b/tests/dialects/test_duckdb.py
index e4788ec116..2127306a4a 100644
--- a/tests/dialects/test_duckdb.py
+++ b/tests/dialects/test_duckdb.py
@@ -858,6 +858,18 @@ def test_duckdb(self):
         self.validate_identity(
             "SELECT COALESCE(*COLUMNS(['a', 'b', 'c'])) AS result FROM (SELECT NULL AS a, 42 AS b, TRUE AS c)"
         )
+        self.validate_all(
+            "SELECT * FROM UNNEST(STRUCT(123 AS a))",
+            write={
+                "redshift": UnsupportedError,
+            },
+        )
+        self.validate_all(
+            "SELECT UNNEST(foo) AS x",
+            write={
+                "redshift": UnsupportedError,
+            },
+        )
 
     def test_array_index(self):
         with self.assertLogs(helper_logger) as cm:


Code review comments:
diff_hunk: @@ -388,13 +389,28 @@ def unnest_sql(self, expression: exp.Unnest) -> str:
             args = expression.expressions
             num_args = len(args)
 
-            if num_args > 1:
+            if not num_args == 1:
                 self.unsupported(f"Unsupported number of arguments in UNNEST: {num_args}")
                 return ""
 
-            arg = self.sql(seq_get(args, 0))
+            if not expression.find_ancestor(exp.From, exp.Join):
+                self.unsupported("Unsupported UNNEST when not used in FROM/JOIN clauses")
+                return ""
+
+            from sqlglot.optimizer.annotate_types import annotate_types
+
+            arg = t.cast(exp.Expression, seq_get(args, 0))
+            annotated_arg = annotate_types(arg)
+            if not annotated_arg.is_type(exp.DataType.Type.UNKNOWN) and not annotated_arg.is_type(
+                exp.DataType.Type.ARRAY
+            ):
+                self.unsupported("Unsupported UNNEST without ARRAY argument")
+                return ""
path: sqlglot/dialects/redshift.py
text: @user1:
I'm not sure about this bit, what triggered the need for this code path? Doesn't redshift have a `SUPER` type that can encode arrays too? Should we leave this out?

@author:
I kept thinking that e.g. in DuckDB we can do `SELECT * FROM UNNEST(<struct expr>)` which will bypass the other checks (as the position of the `UNNEST` is valid in Redshift) but will not work properly due to the non-array expr.

However, the `SUPER` data type does make this more complicated than is needed, so will revert this change

diff_hunk: @@ -388,13 +389,28 @@ def unnest_sql(self, expression: exp.Unnest) -> str:
             args = expression.expressions
             num_args = len(args)
 
-            if num_args > 1:
+            if not num_args == 1:
path: sqlglot/dialects/redshift.py
text: @user1:
```suggestion
            if num_args != 1:
```