Merge branch 'main' into chore/bump-clickhouse-sql-parser-v0.5.4

Collapses the query to a single line to match the other cases in the file
and swaps the tenant's metric name for a placeholder.

go.mod

@@ -4,7 +4,7 @@ go 1.25.7
 
 require (
 	dario.cat/mergo v1.0.2
-	github.com/AfterShip/clickhouse-sql-parser v0.5.3
+	github.com/AfterShip/clickhouse-sql-parser v0.5.4
 	github.com/ClickHouse/clickhouse-go/v2 v2.44.0
 	github.com/DATA-DOG/go-sqlmock v1.5.2
 	github.com/SigNoz/clickhouse-go-mock v0.14.0

go.sum

@@ -66,8 +66,8 @@ dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
 filippo.io/edwards25519 v1.2.0 h1:crnVqOiS4jqYleHd9vaKZ+HKtHfllngJIiOpNpoJsjo=
 filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q77mc=
-github.com/AfterShip/clickhouse-sql-parser v0.5.3 h1:6iap8XGjuSjD3w7r1UNrg66ljBugcv2P39s4eo/ZLRw=
-github.com/AfterShip/clickhouse-sql-parser v0.5.3/go.mod h1:Qi3qvPTfZb/aFwI5V4WFOahgjsLJa4MzVijIAfwOhDw=
+github.com/AfterShip/clickhouse-sql-parser v0.5.4 h1:yiCQaMq8EO+dpKdnpP9YYd/ne6MSuOXgsMsNL33NiTI=
+github.com/AfterShip/clickhouse-sql-parser v0.5.4/go.mod h1:Qi3qvPTfZb/aFwI5V4WFOahgjsLJa4MzVijIAfwOhDw=
 github.com/Azure/azure-sdk-for-go v68.0.0+incompatible h1:fcYLmCpyNYRnvJbPerq7U0hS+6+I79yEDJBqVNcqUzU=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 h1:fou+2+WFTib47nS+nz/ozhEBnvU96bKHy6LjRsY4E28=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0/go.mod h1:t76Ruy8AHvUAC8GfMWJMa0ElSbuIcO03NLpynfbgsPA=

pkg/querybuilder/clickhouse_sql_test.go

@@ -2,12 +2,11 @@ package querybuilder
 
 import (
 	"testing"
+	"time"
 
 	"github.com/SigNoz/signoz/pkg/errors"
 
-	chparser "github.com/AfterShip/clickhouse-sql-parser/parser"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
@@ -21,6 +20,9 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 		{"CommonTableExpression", "WITH t AS (SELECT fingerprint FROM signoz_metrics.time_series_v4) SELECT * FROM t"},
 		{"Join", "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.b"},
 		{"GlobalIn", "SELECT a FROM t WHERE a GLOBAL IN (SELECT b FROM t2)"},
+		// GLOBAL parsed only when the join type was omitted, and only before IN. https://github.com/AfterShip/clickhouse-sql-parser/pull/293
+		{"GlobalLeftJoin", "SELECT * FROM t1 GLOBAL LEFT JOIN t2 ON t1.a = t2.a"},
+		{"GlobalNotIn", "SELECT a FROM t WHERE a GLOBAL NOT IN (SELECT b FROM t2)"},
 		{"Union", "SELECT * FROM t UNION ALL SELECT * FROM t2"},
 		{"Intersect", "SELECT * FROM t INTERSECT SELECT * FROM t2"},
 		{"WindowFunction", "SELECT sum(v) OVER (PARTITION BY a ORDER BY t) FROM t"},
@@ -36,18 +38,37 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 		// order by interval
 		{"OrderByInterval", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval ORDER BY interval"},
 		{"OrderByIntervalAndDirection", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS `interval` ORDER BY `interval` ASC"},
-		// Unspaced, so rejected until the parser stopped lexing a signed literal after a
-		// closing bracket. The spaced form above no longer needs to be spaced.
-		// https://github.com/AfterShip/clickhouse-sql-parser/issues/286
+		// `interval` is a unit keyword, so unquoting it was rejected everywhere the parser
+		// expected a plain identifier. https://github.com/AfterShip/clickhouse-sql-parser/pull/296
+		{"OrderByUnquotedIntervalAsc", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval FROM t GROUP BY interval ORDER BY interval ASC"},
+		{"OrderByUnquotedIntervalDesc", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval FROM t GROUP BY interval ORDER BY interval DESC"},
+		{"UnquotedIntervalInGroupByTuple", "SELECT a FROM t GROUP BY (`service.name`, `service.version`, interval)"},
+		{"UnquotedIntervalProductionQuery", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, resource_string_service$$name AS `service.name`, attributes_string['http.route'] AS `http.route`, quantile(0.95)(duration_nano) / 1000000000 AS value FROM signoz_traces.distributed_signoz_index_v3 WHERE resource_string_service$$name = 'svc-a' AND resources_string['deployment.environment'] = 'dev' AND attributes_string['http.route'] = '/v1' AND http_method = 'POST' AND timestamp BETWEEN toDateTime(1784601720) AND toDateTime(1784602620) AND ts_bucket_start BETWEEN 1784601720 - 1800 AND 1784602620 GROUP BY `service.name`, `http.route`, interval ORDER BY interval ASC"},
+		// Separating the two readings of INTERVAL needs backtracking as per the current implementation which could have performance regressions.
+		// https://github.com/AfterShip/clickhouse-sql-parser/pull/296#issuecomment-5150316367
+		{"UnquotedIntervalRepeatedThirtyTimes", "SELECT interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval AS total FROM t WHERE interval > 0 ORDER BY interval ASC"},
 		{"SignedLiteralAfterClosingParenUnspaced", "SELECT now() AS ts, toFloat64(count()) AS value FROM ( SELECT attributes_string['TableName'] AS T, attributes_string['MissingId'] AS M, max(fromUnixTimestamp64Nano(timestamp)) AS last_seen, dateDiff('minute', min(fromUnixTimestamp64Nano(timestamp)), max(fromUnixTimestamp64Nano(timestamp))) AS age_min FROM signoz_logs.distributed_logs_v2 WHERE body='missing_map_record' AND timestamp >= (toUnixTimestamp(now())-3600)*1000000000 GROUP BY T, M ) WHERE age_min >= 20 AND last_seen >= now() - toIntervalMinute(8)"},
 		{"SignedLiteralAfterClosingParenMinimal", "SELECT (1)-1"},
 		{"TrimFunction", "SELECT trimBoth('/api/endpoint/', '/');"},
+		// The SQL-standard keyword-separated argument forms, which took commas only. https://github.com/AfterShip/clickhouse-sql-parser/pull/290
+		{"StandardTrimSyntax", "SELECT trim(BOTH ' ' FROM body) FROM t"},
+		{"StandardSubstringSyntax", "SELECT substring(body FROM 2 FOR 3) FROM t"},
+		{"StandardOverlaySyntax", "SELECT overlay(body PLACING 'x' FROM 2) FROM t"},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			err := ErrIfStatementIsNotValid(testCase.query)
-			assert.NoError(t, err)
+			// Bounded rather than called directly: a parser that backtracks without memoising
+			// hangs instead of returning. Every case here parses in well under a millisecond.
+			errC := make(chan error, 1)
+			go func() { errC <- ErrIfStatementIsNotValid(testCase.query) }()
+
+			select {
+			case err := <-errC:
+				assert.NoError(t, err)
+			case <-time.After(10 * time.Second):
+				assert.Fail(t, "timed out, which means the parser is no longer bounding its backtracking")
+			}
 		})
 	}
 }
@@ -70,6 +91,8 @@ func TestErrIfStatementIsNotValid_Fail(t *testing.T) {
 		{"CreateTable", "CREATE TABLE evil (a Int) ENGINE = Memory", CodeClickHouseSQLNotSelect},
 		{"Grant", "GRANT ALL ON *.* TO admin", CodeClickHouseSQLNotSelect},
 		{"Set", "SET readonly = 0", CodeClickHouseSQLNotSelect},
+		// The parser still dereferences nil on a DEFAULT expression it cannot read, so the recover is what turns this into a rejection rather than a crash.
+		{"UnparseableDefaultExpression", "CREATE TABLE t (a String DEFAULT foo(b FROM 2)) ENGINE = Memory", CodeClickHouseSQLParserPanic},
 		// These the parser rejects outright rather than classifying.
 		{"ShowGrants", "SHOW GRANTS", CodeClickHouseSQLUnparseable},
 		{"IntoOutfile", "SELECT * FROM t INTO OUTFILE '/tmp/x.csv'", CodeClickHouseSQLUnparseable},
@@ -104,33 +127,19 @@ func TestErrIfStatementIsNotValid_Fail(t *testing.T) {
 	}
 }
 
-// Queries the parser cannot read. ClickHouse runs all of them.
+// Queries ClickHouse runs that this rejects anyway. Each is a known false positive.
 func TestErrIfStatementIsNotValid_ShouldPassButFails(t *testing.T) {
 	testCases := []struct {
-		name  string
-		query string
-		// The construct the parser stops after, which is the one it cannot read.
-		expectedStopsAfter string
-		// The same construct written so the parser accepts it.
-		fix string
+		name         string
+		query        string
+		expectedCode errors.Code
 	}{
 		{
-			name:               "IntervalAliasInOrderBy",
-			query:              "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, resource_string_service$$name AS `service.name`, attributes_string['http.route'] AS `http.route`, quantile(0.95)(duration_nano) / 1000000000 AS value FROM signoz_traces.distributed_signoz_index_v3 WHERE resource_string_service$$name = 'svc-a' AND resources_string['deployment.environment'] = 'dev' AND attributes_string['http.route'] = '/v1' AND http_method = 'POST' AND timestamp BETWEEN toDateTime(1784601720) AND toDateTime(1784602620) AND ts_bucket_start BETWEEN 1784601720 - 1800 AND 1784602620 GROUP BY `service.name`, `http.route`, interval ORDER BY interval ASC",
-			expectedStopsAfter: "ORDER BY interval ASC",
-			fix:                "SELECT count() AS interval FROM t ORDER BY `interval` ASC",
-		},
-		{
-			name:               "IntervalAliasInOrderByDesc",
-			query:              "SELECT count() AS value, toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, serviceName, resourceTagsMap['deployment.environment'] AS environment, exceptionStacktrace FROM signoz_traces.distributed_signoz_error_index_v2 WHERE exceptionType != 'OSError' AND resourceTagsMap['deployment.environment'] = 'staging' AND timestamp BETWEEN toDateTime(1785186300) AND toDateTime(1785186600) GROUP BY serviceName, interval, environment, exceptionStacktrace ORDER BY interval DESC",
-			expectedStopsAfter: "ORDER BY interval DESC",
-			fix:                "SELECT count() AS interval FROM t ORDER BY `interval` DESC",
-		},
-		{
-			name:               "StandardTrimSyntax",
-			query:              "SELECT toStartOfInterval(fromUnixTimestamp64Nano(timestamp), INTERVAL 5 MINUTE) AS interval, resources_string['host.name'] as host_name, toFloat64(countIf( lower(trim(BOTH ' ' FROM replaceOne( JSONExtractString(body, 'Action'), 'health_status: ', '' ))) IN ('unhealthy','starting','failing') )) as value FROM signoz_logs.distributed_logs_v2 WHERE timestamp BETWEEN 1784602320000000000 AND 1784602620000000000 AND ts_bucket_start BETWEEN 1784602320 - 300 AND 1784602620 AND JSONExtractString(body, 'Type') = 'container' AND JSONExtractString(body, 'Actor', 'Attributes', 'name') IS NOT NULL AND resources_string['host.name'] IS NOT NULL AND resources_string['host.name'] = 'aihub-nightly' GROUP BY interval, host_name ORDER BY interval, host_name",
-			expectedStopsAfter: "trim(BOTH '",
-			fix:                "SELECT trimBoth(replaceOne( JSONExtractString(body, 'Action'), 'health_status: ', '' ), ' ')",
+			// numbers() generates rows rather than reading through anything, so the blanket
+			// table-function rule is stricter here than the threat it exists for.
+			name:         "NumbersTableFunction",
+			query:        "SELECT intervals.interval AS interval, active.cluster AS cluster, toFloat64(if(ts_data.has_data = 0, 0, 1)) AS value FROM ( SELECT DISTINCT JSONExtractString(labels, 'k8s.cluster.name') AS cluster FROM signoz_metrics.distributed_time_series_v4 WHERE metric_name = 'my_metric' AND unix_milli >= toUnixTimestamp(now() - INTERVAL 30 DAY) * 1000 HAVING cluster != '' ) AS active CROSS JOIN ( SELECT toStartOfInterval( toDateTime(toUnixTimestamp(now() - INTERVAL 30 MINUTE) + number * 60), INTERVAL 1 MINUTE ) AS interval FROM numbers(31) ) AS intervals LEFT JOIN ( SELECT toStartOfInterval( toDateTime(intDiv(s.unix_milli, 1000)), INTERVAL 1 MINUTE ) AS interval, JSONExtractString(ts.labels, 'k8s.cluster.name') AS cluster, 1 AS has_data FROM signoz_metrics.distributed_samples_v4 s INNER JOIN ( SELECT DISTINCT fingerprint, labels FROM signoz_metrics.distributed_time_series_v4 WHERE metric_name = 'my_metric' ) AS ts ON s.fingerprint = ts.fingerprint WHERE s.metric_name = 'my_metric' AND s.unix_milli >= toUnixTimestamp(now() - INTERVAL 30 MINUTE) * 1000 GROUP BY interval, cluster ) AS ts_data ON active.cluster = ts_data.cluster AND intervals.interval = ts_data.interval ORDER BY interval ASC",
+			expectedCode: CodeClickHouseSQLTableFunction,
 		},
 	}
 
@@ -138,15 +147,8 @@ func TestErrIfStatementIsNotValid_ShouldPassButFails(t *testing.T) {
 		t.Run(testCase.name, func(t *testing.T) {
 			err := ErrIfStatementIsNotValid(testCase.query)
 
-			var parseErr *chparser.ParseError
-			require.ErrorAs(t, err, &parseErr, "expected a parser failure rather than a rule violation")
-
-			// The parser reports the offset it stopped at, which sits just past the construct
-			// it choked on, so the text leading up to it is what needs looking at.
-			consumed := testCase.query[:parseErr.Pos]
-			assert.Equal(t, testCase.expectedStopsAfter, consumed[max(0, len(consumed)-len(testCase.expectedStopsAfter)):])
-
-			assert.NoError(t, ErrIfStatementIsNotValid(testCase.fix))
+			assert.Error(t, err)
+			assert.True(t, errors.Asc(err, testCase.expectedCode), "expected code %s, got %v", testCase.expectedCode, err)
 		})
 	}
 }