Repository: tobymao/sqlglot

Issue:
sqlglot.diff Bug for exp.Window Differences
The following statements are considered equal by `sqlglot.diff`.

However, one is `OVER`, while the other is `KEEP DENSE_RANK`, and thus these should not be considered semantically identical.

```
import sqlglot


parsed = sqlglot.parse_one(
    '''
        SELECT MAX(foo) OVER (
            ORDER BY bar
        )
        FROM baz
    ''',
    dialect='oracle'
)

parsed2 = sqlglot.parse_one(
    '''
        SELECT MAX(foo) KEEP (
            DENSE_RANK LAST
            ORDER BY bar
        )
        FROM baz
    ''',
    dialect='oracle'
)

diffs = sqlglot.diff(parsed, parsed2)

from sqlglot.diff import Keep

# This should fail but does not.
assert all(isinstance(diff, Keep) for diff in diffs)
```

Discussion:
@mkmoisen: @georgesittas Thanks for the quick fix. 

I did have a question about the robustness of `sqlglot.diff` if you don't mind.

Our application uses python to dynamically generate a large number of SQLs which are then stored and later executed. We are currently using sqlparse to format these but it doesn't adhere to our SQL style guidelines, and can be hard to read with lots of nesting.

I'm trying using sqlglot to construct an AST and then pretty print the AST in my preferred SQL style. So far it's been great and I think I'm ready to pull the trigger in our application.

Because I am writing a custom .parsed.sql() function essentially, I am however highly concerned that I will have some bug and will cause a different sql output.

My hope was to simply compare the `sqlparse.diff` between the original sql and the output sql.

Do you think sqlparse.diff is robust enough to account for all differences? Is this something I can rely on for critical production code?

It's not clear to me if this issue was just a rare bug, or if diffing sql AST's are inherently complicated and difficult to achieve with perfect accuracy. 

What do you think?@georgesittas: I guess you meant `sqlglot.diff`, and not `sqlparse.diff`? In any case...

> Do you think sqlparse.diff is robust enough to account for all differences? Is this something I can rely on for critical production code?

I think it's pretty robust, we rely on it in SQLMesh, so we're pretty on top of bugs that may occasionally show up. Diffing _is_ hard though, so there may be some "exotic" cases that don't work well yet. I'd suggest giving it a go anyway, and see how it looks for your case. We'll be happy to look into bugs you may find.

Btw, I see you mentioned `parsed.sql()` in your post. If you're not doing it already, I'd suggest being explicit about what dialect is used to parse and generate SQL, otherwise you may get unexpected behavior because the "base" `Dialect` (i.e. what's used by default) is not as stable as its subclasses.@mkmoisen: @georgesittas 

> I guess you meant sqlglot.diff, and not sqlparse.diff? 

Right, sqlglot.diff.

>  I'd suggest being explicit about what dialect is used

Yep I figured that out yesterday the hard way.

---

Do you know of anyone writing custom SQL formatters using sqlglot?

What I'm doing is basically a recursive function that checks the instance of `expression` and then formats according to our style guidelines.

Essentially something like this:

```
def format_sql(sql):
    parsed = sqlglot.parse_one(sql, dialect='oracle')
    
    def format_expression(expression, level=0):
        indent = "    " * level
        
        if isinstance(expression, exp.Select):
            parts = ['SELECT ']
            if node.args['hint']:
                parts.append(f"{indent}{select} {format_expression(node.args['hint'], level)}")
            ...
            return '\n'.join(parts)
            
        elif isinstance(expression, exp.Union):
            union = 'UNION'
            if not node.args['distinct']:
                union = 'UNION ALL'
                
            return f"{format_expression(expression.this, level)}\n\n{indent}{union}\n\n{format_expression(expression.expression, level)}"
          
        elif isinstance(node, exp.Group):
            parts = ['GROUP BY ']
            parts.append(f",\n{indent}".join(format_expression(expression, level + 1) for expression in expression.expressions))
            return "".join(parts)
    
    ...
    
    return format_expression(parsed)
```


etc. 

Would you do the same or something different?

For example I thought about subclassing the Generator class and making modifications there.@georgesittas: > Do you know of anyone writing custom SQL formatters using sqlglot?

Nope, haven't seen something like that yet. What you're saying is reasonable though, i.e. subclassing & modifying `Generator`. Just keep in mind that there are several dialect overrides that will need to be updated, too.

Solution patch:
diff --git a/sqlglot/diff.py b/sqlglot/diff.py
index 5ebc682e3b..eaca8b8a7d 100644
--- a/sqlglot/diff.py
+++ b/sqlglot/diff.py
@@ -129,12 +129,14 @@ def compute_node_mappings(
 
 # The expression types for which Update edits are allowed.
 UPDATABLE_EXPRESSION_TYPES = (
+    exp.Alias,
     exp.Boolean,
+    exp.Column,
     exp.DataType,
+    exp.Lambda,
     exp.Literal,
     exp.Table,
-    exp.Column,
-    exp.Lambda,
+    exp.Window,
 )
 
 IGNORED_LEAF_EXPRESSION_TYPES = (exp.Identifier,)


Test patch:
diff --git a/tests/test_diff.py b/tests/test_diff.py
index f83c805a30..a83452329f 100644
--- a/tests/test_diff.py
+++ b/tests/test_diff.py
@@ -157,11 +157,20 @@ def test_window_functions(self):
         self._validate_delta_only(
             diff_delta_only(expr_src, expr_tgt),
             [
-                Remove(parse_one("ROW_NUMBER()")),  # the Anonymous node
-                Insert(parse_one("RANK()")),  # the Anonymous node
+                Remove(parse_one("ROW_NUMBER()")),
+                Insert(parse_one("RANK()")),
+                Update(source=expr_src.selects[0], target=expr_tgt.selects[0]),
             ],
         )
 
+        expr_src = parse_one("SELECT MAX(x) OVER (ORDER BY y) FROM z", "oracle")
+        expr_tgt = parse_one("SELECT MAX(x) KEEP (DENSE_RANK LAST ORDER BY y) FROM z", "oracle")
+
+        self._validate_delta_only(
+            diff_delta_only(expr_src, expr_tgt),
+            [Update(source=expr_src.selects[0], target=expr_tgt.selects[0])],
+        )
+
     def test_pre_matchings(self):
         expr_src = parse_one("SELECT 1")
         expr_tgt = parse_one("SELECT 1, 2, 3, 4")
@@ -202,5 +211,19 @@ def test_identifier(self):
             ],
         )
 
+        expr_src = parse_one("SELECT 1 AS c1, 2 AS c2")
+        expr_tgt = parse_one("SELECT 2 AS c1, 3 AS c2")
+
+        self._validate_delta_only(
+            diff_delta_only(expr_src, expr_tgt),
+            [
+                Remove(expression=exp.alias_(1, "c1")),
+                Remove(expression=exp.Literal.number(1)),
+                Insert(expression=exp.alias_(3, "c2")),
+                Insert(expression=exp.Literal.number(3)),
+                Update(source=exp.alias_(2, "c2"), target=exp.alias_(2, "c1")),
+            ],
+        )
+
     def _validate_delta_only(self, actual_delta, expected_delta):
         self.assertEqual(set(actual_delta), set(expected_delta))