Fix wrongly format the ON CLUSTER and LIMIT OFFSET clause (#76)

AfterShip · Jul 15, 2024 · 5c4ca69 · 5c4ca69
1 parent 16c376a
commit 5c4ca69
Show file tree

Hide file tree

Showing 10 changed files with 54 additions and 27 deletions.
diff --git a/parser/ast.go b/parser/ast.go
@@ -1761,7 +1761,7 @@ func (r *RoleName) String(level int) string {
 		builder.WriteString(r.Scope.String(level))
 	}
 	if r.OnCluster != nil {
-		builder.WriteString(" ON ")
+		builder.WriteByte(' ')
 		builder.WriteString(r.OnCluster.String(level))
 	}
 	return builder.String()
@@ -1851,7 +1851,9 @@ func (r *RoleSetting) String(level int) string {
 		builder.WriteString(settingPair.String(level))
 	}
 	if r.Modifier != nil {
-		builder.WriteString(" ")
+		if len(r.SettingPairs) > 0 {
+			builder.WriteString(" ")
+		}
 		builder.WriteString(r.Modifier.String(level))
 	}
 	return builder.String()
@@ -4490,12 +4492,8 @@ func (g *GroupByClause) String(level int) string {
 	builder.WriteString("GROUP BY ")
 	if g.AggregateType != "" {
 		builder.WriteString(g.AggregateType)
-		builder.WriteByte('(')
 	}
 	builder.WriteString(g.Expr.String(level))
-	if g.AggregateType != "" {
-		builder.WriteByte(')')
-	}
 	if g.WithCube {
 		builder.WriteString(" WITH CUBE")
 	}
@@ -4606,10 +4604,7 @@ func (l *LimitByClause) End() Pos {
 
 func (l *LimitByClause) String(level int) string {
 	var builder strings.Builder
-	builder.WriteString("LIMIT ")
-	builder.WriteString(l.Limit.String(level))
 	if l.Limit != nil {
-		builder.WriteString(" OFFSET ")
 		builder.WriteString(l.Limit.String(level))
 	}
 	if l.ByExpr != nil {

diff --git a/parser/parser_table.go b/parser/parser_table.go
@@ -1095,10 +1095,11 @@ func (p *Parser) parseColumnNamesExpr(pos Pos) (*ColumnNamesExpr, error) {
 		if err != nil {
 			return nil, err
 		}
+
+		columnNames = append(columnNames, *name)
 		if p.tryConsumeTokenKind(",") == nil {
 			break
 		}
-		columnNames = append(columnNames, *name)
 	}
 	rightParenPos := p.Pos()
 	if _, err := p.consumeTokenKind(")"); err != nil {

diff --git a/parser/parser_test.go b/parser/parser_test.go
@@ -95,11 +95,15 @@ func TestParser_Format(t *testing.T) {
 				builder.WriteString("-- Origin SQL:\n")
 				builder.Write(fileBytes)
 				builder.WriteString("\n\n-- Format SQL:\n")
+				var formatSQLBuilder strings.Builder
 				for _, stmt := range stmts {
-					builder.WriteString(stmt.String(0))
-					builder.WriteByte(';')
-					builder.WriteByte('\n')
+					formatSQLBuilder.WriteString(stmt.String(0))
+					formatSQLBuilder.WriteByte(';')
+					formatSQLBuilder.WriteByte('\n')
 				}
+				formatSQL := formatSQLBuilder.String()
+				builder.WriteString(formatSQL)
+				validFormatSQL(t, formatSQL)
 				g := goldie.New(t,
 					goldie.WithNameSuffix(""),
 					goldie.WithDiffEngine(goldie.ColoredDiff),
@@ -109,3 +113,17 @@ func TestParser_Format(t *testing.T) {
 		}
 	}
 }
+
+// validFormatSQL Verify that the format sql can be re-parsed with consistent results
+func validFormatSQL(t *testing.T, sql string) {
+	parser := NewParser(sql)
+	stmts, err := parser.ParseStmts()
+	require.NoError(t, err)
+	var builder strings.Builder
+	for _, stmt := range stmts {
+		builder.WriteString(stmt.String(0))
+		builder.WriteByte(';')
+		builder.WriteByte('\n')
+	}
+	require.Equal(t, sql, builder.String())
+}
diff --git a/parser/testdata/ddl/format/alter_role.sql b/parser/testdata/ddl/format/alter_role.sql
@@ -24,9 +24,9 @@ ALTER ROLE r2_01293@'%.myhost.com';
 
 -- Format SQL:
 ALTER ROLE r1_01293;
-ALTER ROLE r1_01293 ON ON CLUSTER cluster_1 RENAME TO r2_01293;
+ALTER ROLE r1_01293 ON CLUSTER cluster_1 RENAME TO r2_01293;
 ALTER ROLE r1_01293 RENAME TO r2_01293, r3_01293 RENAME TO r4_01293;
-ALTER ROLE r1_01293 SETTINGS  NONE;
+ALTER ROLE r1_01293 SETTINGS NONE;
 ALTER ROLE r2_01293 SETTINGS PROFILE 'default';
 ALTER ROLE r3_01293 SETTINGS max_memory_usage 5000000;
 ALTER ROLE r4_01293 SETTINGS max_memory_usage MIN 5000000;
@@ -40,6 +40,6 @@ ALTER ROLE r1_01293 SETTINGS readonly 1;
 ALTER ROLE r2_01293 SETTINGS PROFILE 'default';
 ALTER ROLE r3_01293 SETTINGS max_memory_usage 5000000 MIN 4000000 MAX 6000000 WRITABLE;
 ALTER ROLE r4_01293 SETTINGS PROFILE 'default', max_memory_usage 5000000, readonly 1;
-ALTER ROLE r5_01293 SETTINGS  NONE;
+ALTER ROLE r5_01293 SETTINGS NONE;
 ALTER ROLE r1_01293@'%';
 ALTER ROLE r2_01293@'%.myhost.com';
diff --git a/parser/testdata/ddl/format/create_role.sql b/parser/testdata/ddl/format/create_role.sql
@@ -26,11 +26,11 @@ CREATE ROLE r2_01293@'%.myhost.com';
 
 -- Format SQL:
 CREATE ROLE r1_01293;
-CREATE ROLE r1_01293 ON ON CLUSTER cluster_1;
+CREATE ROLE r1_01293 ON CLUSTER cluster_1;
 CREATE ROLE r1_01293, r2_01293;
-CREATE ROLE r1_01293 ON ON CLUSTER cluster_1, r2_01293;
-CREATE ROLE r1_01293 ON ON CLUSTER cluster_1, r2_01293 ON ON CLUSTER cluster_2;
-CREATE ROLE r1_01293 SETTINGS  NONE;
+CREATE ROLE r1_01293 ON CLUSTER cluster_1, r2_01293;
+CREATE ROLE r1_01293 ON CLUSTER cluster_1, r2_01293 ON CLUSTER cluster_2;
+CREATE ROLE r1_01293 SETTINGS NONE;
 CREATE ROLE r2_01293 SETTINGS PROFILE 'default';
 CREATE ROLE r3_01293 SETTINGS max_memory_usage 5000000;
 CREATE ROLE r4_01293 SETTINGS max_memory_usage MIN 5000000;
@@ -44,6 +44,6 @@ CREATE ROLE r1_01293 SETTINGS readonly 1;
 CREATE ROLE r2_01293 SETTINGS PROFILE 'default';
 CREATE ROLE r3_01293 SETTINGS max_memory_usage 5000000 MIN 4000000 MAX 6000000 WRITABLE;
 CREATE ROLE r4_01293 SETTINGS PROFILE 'default', max_memory_usage 5000000, readonly 1;
-CREATE ROLE r5_01293 SETTINGS  NONE;
+CREATE ROLE r5_01293 SETTINGS NONE;
 CREATE ROLE r1_01293@'%';
 CREATE ROLE r2_01293@'%.myhost.com';
diff --git a/parser/testdata/dml/format/insert_values.sql b/parser/testdata/dml/format/insert_values.sql
@@ -7,7 +7,7 @@ INSERT INTO helloworld.my_first_table (user_id, message, timestamp, metric) VALU
 
 -- Format SQL:
 INSERT INTO TABLE helloworld.my_first_table
-  (user_id, message, timestamp)
+  (user_id, message, timestamp, metric)
 VALUES 
   (101, 'Hello, ClickHouse!', now(), -1.0),
   (102, 'Insert a lot of rows per batch', yesterday(), 1.41421),

diff --git a/parser/testdata/dml/output/insert_values.sql.golden.json b/parser/testdata/dml/output/insert_values.sql.golden.json
@@ -46,6 +46,15 @@
             "NameEnd": 66
           },
           "DotIdent": null
+        },
+        {
+          "Ident": {
+            "Name": "metric",
+            "QuoteType": 1,
+            "NamePos": 68,
+            "NameEnd": 74
+          },
+          "DotIdent": null
         }
       ]
     },

diff --git a/parser/testdata/query/format/select_simple.sql b/parser/testdata/query/format/select_simple.sql
@@ -24,4 +24,4 @@ FROM
 WHERE
   (f0 IN ('foo', 'bar', 'test')) AND (f1 = 'testing') AND (f2 NOT LIKE 'testing2') AND f3 NOT IN ('a', 'b', 'c')
 GROUP BY f0, f1
-LIMIT LIMIT 10 OFFSET 100 OFFSET LIMIT 10 OFFSET 100 BY f0;
+LIMIT 10 OFFSET 100 BY f0;
diff --git a/parser/testdata/query/format/select_simple_with_group_by_with_cube_totals.sql b/parser/testdata/query/format/select_simple_with_group_by_with_cube_totals.sql
@@ -8,5 +8,5 @@ SELECT
   COUNT(b)
 FROM
   group_by_all
-GROUP BY CUBE((a)) WITH CUBE WITH TOTALS
+GROUP BY CUBE(a) WITH CUBE WITH TOTALS
 ORDER BY a;
diff --git a/parser/visitor_test.go b/parser/visitor_test.go
@@ -35,14 +35,18 @@ func TestVisitor_Identical(t *testing.T) {
 				builder.WriteString("-- Origin SQL:\n")
 				builder.Write(fileBytes)
 				builder.WriteString("\n\n-- Format SQL:\n")
+				var formatSQLBuilder strings.Builder
 				for _, stmt := range stmts {
 					err := stmt.Accept(&visitor)
 					require.NoError(t, err)
 
-					builder.WriteString(stmt.String(0))
-					builder.WriteByte(';')
-					builder.WriteByte('\n')
+					formatSQLBuilder.WriteString(stmt.String(0))
+					formatSQLBuilder.WriteByte(';')
+					formatSQLBuilder.WriteByte('\n')
 				}
+				formatSQL := formatSQLBuilder.String()
+				builder.WriteString(formatSQL)
+				validFormatSQL(t, formatSQL)
 				g := goldie.New(t,
 					goldie.WithNameSuffix(""),
 					goldie.WithDiffEngine(goldie.ColoredDiff),