LYT-515 | Lex unquoted negative numeric literals as signed literals in FilterQL

lex/dialect_filterql_test.go

@@ -239,3 +239,56 @@ func TestFilterQLIntersects(t *testing.T) {
 			tv(TokenRightParenthesis, ")"),
 		})
 }
+
+// An unquoted negative numeric literal in a value position must lex as a
+// single signed TokenInteger/TokenFloat, not a TokenMinus followed by a
+// positive number.
+func TestFilterQLNegativeLiteral(t *testing.T) {
+	verifyFilterQLTokens(t, `FILTER visitct = -1`,
+		[]Token{
+			tv(TokenFilter, "FILTER"),
+			tv(TokenIdentity, "visitct"),
+			tv(TokenEqual, "="),
+			tv(TokenInteger, "-1"),
+		})
+
+	verifyFilterQLTokens(t, `FILTER visitct = -1.5`,
+		[]Token{
+			tv(TokenFilter, "FILTER"),
+			tv(TokenIdentity, "visitct"),
+			tv(TokenEqual, "="),
+			tv(TokenFloat, "-1.5"),
+		})
+
+	verifyFilterQLTokens(t, `FILTER visitct IN (-1)`,
+		[]Token{
+			tv(TokenFilter, "FILTER"),
+			tv(TokenIdentity, "visitct"),
+			tv(TokenIN, "IN"),
+			tv(TokenLeftParenthesis, "("),
+			tv(TokenInteger, "-1"),
+			tv(TokenRightParenthesis, ")"),
+		})
+
+	verifyFilterQLTokens(t, `FILTER visitct IN (-1, 3)`,
+		[]Token{
+			tv(TokenFilter, "FILTER"),
+			tv(TokenIdentity, "visitct"),
+			tv(TokenIN, "IN"),
+			tv(TokenLeftParenthesis, "("),
+			tv(TokenInteger, "-1"),
+			tv(TokenComma, ","),
+			tv(TokenInteger, "3"),
+			tv(TokenRightParenthesis, ")"),
+		})
+
+	verifyFilterQLTokens(t, `FILTER city IN (-1)`,
+		[]Token{
+			tv(TokenFilter, "FILTER"),
+			tv(TokenIdentity, "city"),
+			tv(TokenIN, "IN"),
+			tv(TokenLeftParenthesis, "("),
+			tv(TokenInteger, "-1"),
+			tv(TokenRightParenthesis, ")"),
+		})
+}

lex/lexer.go

@@ -1291,6 +1291,13 @@ func LexListOfArgs(l *Lexer) StateFn {
 		l.backup()
 		return LexExpression
 	case '!', '=', '>', '<', '-', '+', '%', '&', '/', '|':
+		if r == '-' && valueExpectedTokens[l.lastToken.T] && l.numericAfterSign() {
+			// A negative literal is a single value, not a binary operator
+			// between two list args: push back onto this list so the
+			// following `,` / `)` is handled here, not by the enclosing
+			// LexParenRight.
+			l.Push("LexListOfArgs", LexListOfArgs)
+		}
 		l.backup()
 		return LexExpression
 	case ';':
@@ -2244,6 +2251,47 @@ func LexLogical(l *Lexer) StateFn {
 	return LexExpression(l)
 }
 
+// valueExpectedTokens are the previously-emitted tokens after which an
+// unquoted `-` begins a signed numeric literal rather than the binary-minus
+// operator: comparators, arithmetic operators, open-paren, comma, logic,
+// IN/BETWEEN, and the start of input (TokenNil).
+var valueExpectedTokens = map[TokenType]bool{
+	TokenNil:             true,
+	TokenEqual:           true,
+	TokenEqualEqual:      true,
+	TokenNE:              true,
+	TokenGE:              true,
+	TokenLE:              true,
+	TokenGT:              true,
+	TokenLT:              true,
+	TokenMinus:           true,
+	TokenPlus:            true,
+	TokenMultiply:        true,
+	TokenDivide:          true,
+	TokenModulus:         true,
+	TokenLeftParenthesis: true,
+	TokenComma:           true,
+	TokenLogicAnd:        true,
+	TokenLogicOr:         true,
+	TokenAnd:             true,
+	TokenOr:              true,
+	TokenIN:              true,
+	TokenBetween:         true,
+}
+
+// numericAfterSign reports whether the upcoming runes (immediately after an
+// already-consumed sign) are a digit, or a `.` followed by a digit.
+func (l *Lexer) numericAfterSign() bool {
+	next := l.PeekX(2)
+	if len(next) == 0 {
+		return false
+	}
+	if isDigit(rune(next[0])) {
+		return true
+	}
+	return next[0] == '.' && len(next) == 2 && isDigit(rune(next[1]))
+}
+
 // <expr>   Handle single logical expression which may be nested and  has
 //
 //	user defined function names that are NOT validated by lexer
@@ -2313,13 +2361,17 @@ func LexExpression(l *Lexer) StateFn {
 		foundLogical := false
 		foundOperator := false
 		switch r {
-		case '-': // comment?  or minus?
+		case '-': // negative numeric literal, comment, or minus?
 			p := l.Peek()
-			if p == '-' {
+			switch {
+			case p == '-':
 				l.backup()
 				l.Push("LexExpression", LexExpression)
 				return LexInlineComment
-			} else {
+			case valueExpectedTokens[l.lastToken.T] && l.numericAfterSign():
+				l.backup()
+				return LexNumber(l)
+			default:
 				l.Emit(TokenMinus)
 				return l.clauseState()
 			}

lex/lexer_test.go

@@ -270,6 +270,31 @@ func TestLexDuration(t *testing.T) {
 	}
 }
 
+// Binary minus (subtraction) must be unaffected by the signed-numeric-literal
+// fix: LexExpression is shared with the SQL dialect, and `a - b` / `5 - 3`
+// have an identity/number as the previous token, not a value-expected one.
+func TestLexBinaryMinusUnchanged(t *testing.T) {
+	verifyTokens(t, `SELECT a - b FROM x`,
+		[]Token{
+			tv(TokenSelect, "SELECT"),
+			tv(TokenIdentity, "a"),
+			tv(TokenMinus, "-"),
+			tv(TokenIdentity, "b"),
+			tv(TokenFrom, "FROM"),
+			tv(TokenIdentity, "x"),
+		})
+
+	verifyTokens(t, `SELECT 5 - 3 FROM x`,
+		[]Token{
+			tv(TokenSelect, "SELECT"),
+			tv(TokenInteger, "5"),
+			tv(TokenMinus, "-"),
+			tv(TokenInteger, "3"),
+			tv(TokenFrom, "FROM"),
+			tv(TokenIdentity, "x"),
+		})
+}
+
 func verifyTokens(t *testing.T, sql string, tokens []Token) {
 	l := NewSqlLexer(sql)
 	u.Debugf("sql: %v", sql)

rel/parse_filterql_test.go

@@ -100,6 +100,14 @@ var FilterTests = []string{
         LIMIT 100
         -- and some more
     `,
+	// Unquoted negative numeric literals (LYT-515): must round-trip like any
+	// other value literal, on both int- and string-named fields.
+	`FILTER visitct = -1`,
+	`FILTER visitct = -1.5`,
+	`FILTER city = -1`,
+	`FILTER visitct IN (-1)`,
+	`FILTER visitct IN (-1, 3)`,
+	`FILTER city IN (-1)`,
 }
 
 func init() {
@@ -227,6 +235,106 @@ func TestFilterQlRoundTrip(t *testing.T) {
 	}
 }
 
+// numberNodesOf extracts the *expr.NumberNode(s) from a comparison's RHS,
+// which is either a bare NumberNode (`=`) or an ArrayNode of them (`IN`).
+func numberNodesOf(t *testing.T, rhs expr.Node) []*expr.NumberNode {
+	t.Helper()
+	switch rhs := rhs.(type) {
+	case *expr.NumberNode:
+		return []*expr.NumberNode{rhs}
+	case *expr.ArrayNode:
+		nums := make([]*expr.NumberNode, len(rhs.Args))
+		for i, arg := range rhs.Args {
+			n, ok := arg.(*expr.NumberNode)
+			require.True(t, ok, "expected *expr.NumberNode array element, got %T", arg)
+			nums[i] = n
+		}
+		return nums
+	default:
+		t.Fatalf("expected *expr.NumberNode or *expr.ArrayNode, got %T", rhs)
+		return nil
+	}
+}
+
+func TestFilterQLNegativeLiterals(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		ql       string
+		wantText []string
+	}{
+		{`FILTER visitct = -1 FROM user`, []string{"-1"}},
+		{`FILTER city = -1 FROM user`, []string{"-1"}},
+		{`FILTER visitct = -1.5 FROM user`, []string{"-1.5"}},
+		{`FILTER visitct IN (-1) FROM user`, []string{"-1"}},
+		{`FILTER visitct IN (-1, 3) FROM user`, []string{"-1", "3"}},
+		{`FILTER city IN (-1) FROM user`, []string{"-1"}},
+	}
+
+	for _, tc := range tests {
+		req, err := rel.ParseFilterQL(tc.ql)
+		require.NoError(t, err, "must parse %s", tc.ql)
+
+		bn, ok := req.Filter.(*expr.BinaryNode)
+		require.True(t, ok, "expected *expr.BinaryNode for %s, got %T", tc.ql, req.Filter)
+
+		nums := numberNodesOf(t, bn.Args[1])
+		require.Len(t, nums, len(tc.wantText), "element count for %s", tc.ql)
+		for i, n := range nums {
+			assert.Equal(t, tc.wantText[i], n.Text, "signed literal text for %s", tc.ql)
+		}
+
+		// The canonical form must be bare (e.g. `-1`), never `- (1)`, and
+		// re-parsing it must reproduce the same canonical string.
+		out := req.String()
+		assert.Equal(t, tc.ql, out, "canonical form for %s", tc.ql)
+		req2, err := rel.ParseFilterQL(out)
+		require.NoError(t, err, "must reparse canonical form %q", out)
+		assert.Equal(t, out, req2.String(), "round-trip must be idempotent for %s", tc.ql)
+	}
+
+	// Positive and quoted forms must be unaffected.
+	req, err := rel.ParseFilterQL(`FILTER visitct = 1 FROM user`)
+	require.NoError(t, err)
+	bn := req.Filter.(*expr.BinaryNode)
+	n, ok := bn.Args[1].(*expr.NumberNode)
+	require.True(t, ok, "expected *expr.NumberNode, got %T", bn.Args[1])
+	assert.Equal(t, "1", n.Text)
+
+	req, err = rel.ParseFilterQL(`FILTER visitct = "-1" FROM user`)
+	require.NoError(t, err)
+	bn = req.Filter.(*expr.BinaryNode)
+	sn, ok := bn.Args[1].(*expr.StringNode)
+	require.True(t, ok, "expected *expr.StringNode, got %T", bn.Args[1])
+	assert.Equal(t, "-1", sn.Text)
+}
+
+// TestFilterQLNegativeLiteralDirectASTRoundTrip covers the stored-QL
+// round-trip bug from the ticket: a NumberNode{Text:"-1"} built directly
+// (as expr.NodeFromExpr would from a stored JSON AST, bypassing the lexer)
+// must print bare `-1` and that printed form must re-parse to an equivalent
+// NumberNode.
+func TestFilterQLNegativeLiteralDirectASTRoundTrip(t *testing.T) {
+	t.Parallel()
+
+	num, err := expr.NewNumberStr("-1")
+	require.NoError(t, err)
+
+	fs := rel.NewFilterStatement()
+	fs.Filter = expr.NewBinaryNode(lex.Token{T: lex.TokenEqual, V: "="}, expr.NewIdentityNodeVal("visitct"), num)
+
+	out := fs.String()
+	assert.Equal(t, `FILTER visitct = -1`, out)
+
+	req2, err := rel.ParseFilterQL(out)
+	require.NoError(t, err, "must reparse %q", out)
+	bn2, ok := req2.Filter.(*expr.BinaryNode)
+	require.True(t, ok, "expected *expr.BinaryNode, got %T", req2.Filter)
+	n2, ok := bn2.Args[1].(*expr.NumberNode)
+	require.True(t, ok, "expected *expr.NumberNode, got %T", bn2.Args[1])
+	assert.Equal(t, "-1", n2.Text)
+}
+
 func TestFilterQlFingerPrint(t *testing.T) {
 	t.Parallel()