From e136cbab2965abdebd1863a320afcb233064b9e9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 09:22:30 +0000 Subject: [PATCH] Lex unquoted negative numeric literals as signed literals FilterQL rejected unquoted `-1`/`-1.5` in `=` comparisons and hard-failed on them in `IN (...)` lists, while the quoted form parsed fine. Gate the lexer's `-` handling on lastToken so a `-` in a value position (after a comparator, operator, `(`, `,`, logic, IN/BETWEEN, or start-of-input) lexes as one signed TokenInteger/TokenFloat instead of TokenMinus, so it parses to a single *expr.NumberNode and round-trips to a bare `-1` instead of `- (1)`. Binary subtraction is unaffected. Co-authored-by: Onkar Jaliminche Co-authored-by: Vedant Karle Co-authored-by: Claude --- lex/dialect_filterql_test.go | 53 +++++++++++++++++ lex/lexer.go | 58 ++++++++++++++++++- lex/lexer_test.go | 25 ++++++++ rel/parse_filterql_test.go | 108 +++++++++++++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 3 deletions(-) diff --git a/lex/dialect_filterql_test.go b/lex/dialect_filterql_test.go index 771148c5..d353d702 100644 --- a/lex/dialect_filterql_test.go +++ b/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, ")"), + }) +} diff --git a/lex/lexer.go b/lex/lexer.go index ee007f26..0835b6af 100644 --- a/lex/lexer.go +++ b/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])) +} + // 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() } diff --git a/lex/lexer_test.go b/lex/lexer_test.go index 7b151516..9955c24f 100644 --- a/lex/lexer_test.go +++ b/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) diff --git a/rel/parse_filterql_test.go b/rel/parse_filterql_test.go index 998e4f69..2c116b9f 100644 --- a/rel/parse_filterql_test.go +++ b/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()