From 8a04002c706008790524f3c54315e3140946992f Mon Sep 17 00:00:00 2001 From: tdakkota Date: Wed, 2 Aug 2023 05:49:39 +0300 Subject: [PATCH] feat(traceql): define error types with position --- internal/traceql/errors.go | 28 ++++++ internal/traceql/field_expr.go | 2 +- internal/traceql/parser.go | 20 +++-- internal/traceql/parser_field_expr.go | 18 ++-- internal/traceql/parser_pipeline.go | 12 ++- internal/traceql/parser_scalar_expr.go | 15 ++-- internal/traceql/parser_test.go | 88 ++----------------- internal/traceql/scalar_expr.go | 2 +- internal/traceql/type.go | 38 +++++--- internal/traceql/type_test.go | 117 +++++++++++++++++++++++++ 10 files changed, 222 insertions(+), 118 deletions(-) create mode 100644 internal/traceql/errors.go create mode 100644 internal/traceql/type_test.go diff --git a/internal/traceql/errors.go b/internal/traceql/errors.go new file mode 100644 index 00000000..1e1eb3e9 --- /dev/null +++ b/internal/traceql/errors.go @@ -0,0 +1,28 @@ +package traceql + +import ( + "fmt" + "text/scanner" +) + +// SyntaxError is a syntax error. +type SyntaxError struct { + Msg string + Pos scanner.Position +} + +// Error implements error. +func (e *SyntaxError) Error() string { + return fmt.Sprintf("at %s: %s", e.Pos, e.Msg) +} + +// TypeError is a type checking error. +type TypeError struct { + Msg string + Pos scanner.Position +} + +// Error implements error. +func (e *TypeError) Error() string { + return fmt.Sprintf("at %s: %s", e.Pos, e.Msg) +} diff --git a/internal/traceql/field_expr.go b/internal/traceql/field_expr.go index e32ebbb8..8a30fcb5 100644 --- a/internal/traceql/field_expr.go +++ b/internal/traceql/field_expr.go @@ -3,7 +3,7 @@ package traceql // FieldExpr is a field expression. type FieldExpr interface { fieldExpr() - ValueType() StaticType + TypedExpr } func (*BinaryFieldExpr) fieldExpr() {} diff --git a/internal/traceql/parser.go b/internal/traceql/parser.go index 2e8c734d..b0f84045 100644 --- a/internal/traceql/parser.go +++ b/internal/traceql/parser.go @@ -1,6 +1,7 @@ package traceql import ( + "fmt" "strconv" "time" @@ -60,15 +61,24 @@ func (p *parser) unread() { func (p *parser) unexpectedToken(t lexer.Token) error { if t.Type == lexer.EOF { - return errors.New("unexpected EOF") + return &SyntaxError{ + Msg: "unexpected EOF", + Pos: t.Pos, + } + } + return &SyntaxError{ + Msg: fmt.Sprintf("unexpected token %q", t.Type), + Pos: t.Pos, } - return errors.Errorf("unexpected token %q at %s", t.Type, t.Pos) } -func (p *parser) consumeText(tt lexer.TokenType) (string, error) { +func (p *parser) consumeText(expect lexer.TokenType) (string, error) { t := p.next() - if t.Type != tt { - return "", errors.Wrapf(p.unexpectedToken(t), "expected %q", tt) + if t.Type != expect { + return "", &SyntaxError{ + Msg: fmt.Sprintf("expected %q, got %q", expect, t.Type), + Pos: t.Pos, + } } return t.Text, nil } diff --git a/internal/traceql/parser_field_expr.go b/internal/traceql/parser_field_expr.go index 9c42cdcd..e4f17f0e 100644 --- a/internal/traceql/parser_field_expr.go +++ b/internal/traceql/parser_field_expr.go @@ -1,9 +1,9 @@ package traceql import ( + "fmt" "strings" - "github.com/go-faster/errors" "go.opentelemetry.io/collector/pdata/ptrace" "github.com/go-faster/oteldb/internal/traceql/lexer" @@ -48,10 +48,12 @@ func (p *parser) parseFieldExpr1() (FieldExpr, error) { op = OpNeg } - if err := CheckUnaryExpr(op, expr); err != nil { - return nil, errors.Wrapf(err, "at %s", pos) + if t := expr.ValueType(); !op.CheckType(t) { + return nil, &TypeError{ + Msg: fmt.Sprintf("unary operator %q not defined on %q", op, t), + Pos: pos, + } } - return &UnaryFieldExpr{ Expr: expr, Op: op, @@ -95,15 +97,17 @@ func (p *parser) parseBinaryFieldExpr(left FieldExpr, minPrecedence int) (FieldE if !ok || op.Precedence() < minPrecedence { return left, nil } - // Consume op. - p.next() + // Consume op and get op token position. + opPos := p.next().Pos + // Get right expression position. + rightPos := p.peek().Pos right, err := p.parseFieldExpr1() if err != nil { return nil, err } - if err := CheckBinaryExpr(left, op, right); err != nil { + if err := p.checkBinaryExpr(left, op, opPos, right, rightPos); err != nil { return nil, err } diff --git a/internal/traceql/parser_pipeline.go b/internal/traceql/parser_pipeline.go index a4210b79..cad94e1a 100644 --- a/internal/traceql/parser_pipeline.go +++ b/internal/traceql/parser_pipeline.go @@ -1,8 +1,6 @@ package traceql import ( - "github.com/go-faster/errors" - "github.com/go-faster/oteldb/internal/traceql/lexer" ) @@ -39,7 +37,10 @@ func (p *parser) parsePipeline() (stages []PipelineStage, rerr error) { stages = append(stages, op) case lexer.Coalesce: if len(stages) < 1 { - return stages, errors.Errorf("coalesce cannot be first operation: at %s", t.Pos) + return stages, &SyntaxError{ + Msg: "coalesce cannot be first operation", + Pos: t.Pos, + } } p.next() @@ -143,7 +144,10 @@ func (p *parser) parseSpansetExpr1() (SpansetExpr, error) { switch fieldExpr.ValueType() { case TypeBool, TypeAttribute: default: - return nil, errors.Errorf("filter expression must evaluate to boolean: at %s", t2.Pos) + return nil, &TypeError{ + Msg: "filter expression must evaluate to boolean", + Pos: t2.Pos, + } } filter.Expr = fieldExpr } else { diff --git a/internal/traceql/parser_scalar_expr.go b/internal/traceql/parser_scalar_expr.go index a6f50c02..b78ab260 100644 --- a/internal/traceql/parser_scalar_expr.go +++ b/internal/traceql/parser_scalar_expr.go @@ -1,8 +1,6 @@ package traceql import ( - "github.com/go-faster/errors" - "github.com/go-faster/oteldb/internal/traceql/lexer" ) @@ -75,7 +73,10 @@ func (p *parser) parseAggregateScalarExpr() (expr *AggregateScalarExpr, _ error) expr.Field = field if t := field.ValueType(); t != TypeAttribute && !t.IsNumeric() { - return nil, errors.Errorf("aggregate expression must evaluate to numeric: at %s", pos) + return nil, &TypeError{ + Msg: "aggregate expression must evaluate to numeric", + Pos: pos, + } } } @@ -92,15 +93,17 @@ func (p *parser) parseBinaryScalarExpr(left ScalarExpr, minPrecedence int) (Scal if !ok || !op.IsArithmetic() || op.Precedence() < minPrecedence { return left, nil } - // Consume op. - p.next() + // Consume op and get op token position. + opPos := p.next().Pos + // Get right expression position. + rightPos := p.peek().Pos right, err := p.parseScalarExpr1() if err != nil { return nil, err } - if err := CheckBinaryExpr(left, op, right); err != nil { + if err := p.checkBinaryExpr(left, op, opPos, right, rightPos); err != nil { return nil, err } diff --git a/internal/traceql/parser_test.go b/internal/traceql/parser_test.go index 264e2715..e5d313bf 100644 --- a/internal/traceql/parser_test.go +++ b/internal/traceql/parser_test.go @@ -823,85 +823,6 @@ var tests = []TestCase{ {`coalesce()`, nil, true}, // Scalar filter must be a part of pipeline. {`max() > 3 && { }`, nil, true}, - - // Type check errors. - // Spanset expression must evaluate to boolean. - // Static. - {`{ "foo" }`, nil, true}, - {`{ 1 }`, nil, true}, - {`{ 3.14 }`, nil, true}, - {`{ nil }`, nil, true}, - {`{ 5h }`, nil, true}, - {`{ ok }`, nil, true}, - {`{ client }`, nil, true}, - // Attribute. - {`{ duration }`, nil, true}, - {`{ childCount }`, nil, true}, - {`{ name }`, nil, true}, - {`{ status }`, nil, true}, - {`{ kind }`, nil, true}, - {`{ parent }`, nil, true}, - {`{ rootName }`, nil, true}, - {`{ rootServiceName }`, nil, true}, - {`{ traceDuration }`, nil, true}, - // Aggregate expression must evaluate to number. - {`{ .a } | sum(true) > 10`, nil, true}, - {`{ .a } | sum("foo") > 10`, nil, true}, - // Illegal operand. - {`{ -"foo" =~ "foo" }`, nil, true}, - {`{ !"foo" =~ "foo" }`, nil, true}, - {`{ 1 + "foo" }`, nil, true}, - {`{ 1 + true }`, nil, true}, - {`{ 1 + nil }`, nil, true}, - {`{ 1 + ok }`, nil, true}, - {`{ 1 + client }`, nil, true}, - {`{ "foo" > 10 }`, nil, true}, - {`{ "foo" =~ 10 }`, nil, true}, - // Illegal operation. - // String. - {`{ "foo" && "foo" }`, nil, true}, - {`{ "foo" || "foo" }`, nil, true}, - {`{ "foo" + "foo" }`, nil, true}, - {`{ "foo" % "foo" }`, nil, true}, - // Int. - {`{ 1 && 1 }`, nil, true}, - {`{ 1 || 1 }`, nil, true}, - {`{ 1 =~ 1 }`, nil, true}, - {`{ 1 !~ 1 }`, nil, true}, - // Number. - {`{ 3.14 && 3.14 }`, nil, true}, - {`{ 3.14 || 3.14 }`, nil, true}, - {`{ 3.14 =~ 3.14 }`, nil, true}, - {`{ 3.14 !~ 3.14 }`, nil, true}, - // Bool. - {`{ true / true }`, nil, true}, - {`{ true < true }`, nil, true}, - {`{ true =~ true }`, nil, true}, - {`{ true !~ true }`, nil, true}, - // Nil. - {`{ nil - nil }`, nil, true}, - {`{ nil =~ nil }`, nil, true}, - {`{ nil !~ nil }`, nil, true}, - {`{ nil < nil }`, nil, true}, - // Duration. - {`{ 5h && 5h }`, nil, true}, - {`{ 5h || 5h }`, nil, true}, - {`{ 5h =~ 5h }`, nil, true}, - {`{ 5h !~ 5h }`, nil, true}, - // Status. - {`{ ok && ok }`, nil, true}, - {`{ ok || ok }`, nil, true}, - {`{ ok ^ ok }`, nil, true}, - {`{ ok =~ ok }`, nil, true}, - {`{ ok !~ ok }`, nil, true}, - {`{ ok <= ok }`, nil, true}, - // Kind. - {`{ client && client }`, nil, true}, - {`{ client || client }`, nil, true}, - {`{ client * client }`, nil, true}, - {`{ client =~ client }`, nil, true}, - {`{ client !~ client }`, nil, true}, - {`{ client >= client }`, nil, true}, } func TestParse(t *testing.T) { @@ -915,8 +836,12 @@ func TestParse(t *testing.T) { }() got, err := Parse(tt.input) + if err != nil { + t.Logf("Error: \n%+v", err) + } if tt.wantErr { - require.Error(t, err) + var se *SyntaxError + require.ErrorAs(t, err, &se) return } require.NoError(t, err) @@ -929,6 +854,9 @@ func FuzzParse(f *testing.F) { for _, tt := range tests { f.Add(tt.input) } + for _, tt := range typeTests { + f.Add(tt.input) + } f.Fuzz(func(t *testing.T, input string) { defer func() { if r := recover(); r != nil || t.Failed() { diff --git a/internal/traceql/scalar_expr.go b/internal/traceql/scalar_expr.go index 377951a6..26426222 100644 --- a/internal/traceql/scalar_expr.go +++ b/internal/traceql/scalar_expr.go @@ -3,7 +3,7 @@ package traceql // ScalarExpr is a scalar expression. type ScalarExpr interface { scalarExpr() - ValueType() StaticType + TypedExpr } func (*BinaryScalarExpr) scalarExpr() {} diff --git a/internal/traceql/type.go b/internal/traceql/type.go index 027424df..336cea54 100644 --- a/internal/traceql/type.go +++ b/internal/traceql/type.go @@ -1,32 +1,42 @@ package traceql -import "github.com/go-faster/errors" +import ( + "fmt" + "text/scanner" +) // TypedExpr is an interface for typed expression. type TypedExpr interface { ValueType() StaticType } -// CheckBinaryExpr typechecks binary expression. -func CheckBinaryExpr(left TypedExpr, op BinaryOp, right TypedExpr) error { +// checkBinaryExpr typechecks binary expression. +func (p *parser) checkBinaryExpr( + left TypedExpr, + op BinaryOp, + opPos scanner.Position, + right TypedExpr, + rightPos scanner.Position, +) error { lt := left.ValueType() rt := right.ValueType() if !op.CheckType(lt) { - return errors.Errorf("can't apply op %q to %q", op, lt) + return &TypeError{ + Msg: fmt.Sprintf("binary operator %q not defined on %q", op, lt), + Pos: opPos, + } } if !op.CheckType(rt) { - return errors.Errorf("can't apply op %q to %q", op, rt) + return &TypeError{ + Msg: fmt.Sprintf("binary operator %q not defined on %q", op, rt), + Pos: opPos, + } } if !lt.CheckOperand(rt) { - return errors.Errorf("invalid operand types: %q and %q", lt, rt) - } - return nil -} - -// CheckUnaryExpr typechecks unary expression. -func CheckUnaryExpr(op UnaryOp, expr TypedExpr) error { - if t := expr.ValueType(); !op.CheckType(t) { - return errors.Errorf("can't apply op %q to %q", op, t) + return &TypeError{ + Msg: fmt.Sprintf("operand types mismatch: %q and %q", lt, rt), + Pos: rightPos, + } } return nil } diff --git a/internal/traceql/type_test.go b/internal/traceql/type_test.go new file mode 100644 index 00000000..ff0072d3 --- /dev/null +++ b/internal/traceql/type_test.go @@ -0,0 +1,117 @@ +package traceql + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +var typeTests = []struct { + input string + want Expr + wantErr bool +}{ + // Spanset expression must evaluate to boolean. + // Static. + {`{ "foo" }`, nil, true}, + {`{ 1 }`, nil, true}, + {`{ 3.14 }`, nil, true}, + {`{ nil }`, nil, true}, + {`{ 5h }`, nil, true}, + {`{ ok }`, nil, true}, + {`{ client }`, nil, true}, + // Attribute. + {`{ duration }`, nil, true}, + {`{ childCount }`, nil, true}, + {`{ name }`, nil, true}, + {`{ status }`, nil, true}, + {`{ kind }`, nil, true}, + {`{ parent }`, nil, true}, + {`{ rootName }`, nil, true}, + {`{ rootServiceName }`, nil, true}, + {`{ traceDuration }`, nil, true}, + // Aggregate expression must evaluate to number. + {`{ .a } | sum(true) > 10`, nil, true}, + {`{ .a } | sum("foo") > 10`, nil, true}, + // Illegal operand. + {`{ -"foo" =~ "foo" }`, nil, true}, + {`{ !"foo" =~ "foo" }`, nil, true}, + {`{ 1 + "foo" }`, nil, true}, + {`{ 1 + true }`, nil, true}, + {`{ 1 + nil }`, nil, true}, + {`{ 1 + ok }`, nil, true}, + {`{ 1 + client }`, nil, true}, + {`{ "foo" > 10 }`, nil, true}, + {`{ "foo" =~ 10 }`, nil, true}, + // Illegal operation. + // String. + {`{ "foo" && "foo" }`, nil, true}, + {`{ "foo" || "foo" }`, nil, true}, + {`{ "foo" + "foo" }`, nil, true}, + {`{ "foo" % "foo" }`, nil, true}, + // Int. + {`{ 1 && 1 }`, nil, true}, + {`{ 1 || 1 }`, nil, true}, + {`{ 1 =~ 1 }`, nil, true}, + {`{ 1 !~ 1 }`, nil, true}, + // Number. + {`{ 3.14 && 3.14 }`, nil, true}, + {`{ 3.14 || 3.14 }`, nil, true}, + {`{ 3.14 =~ 3.14 }`, nil, true}, + {`{ 3.14 !~ 3.14 }`, nil, true}, + // Bool. + {`{ true / true }`, nil, true}, + {`{ true < true }`, nil, true}, + {`{ true =~ true }`, nil, true}, + {`{ true !~ true }`, nil, true}, + // Nil. + {`{ nil - nil }`, nil, true}, + {`{ nil =~ nil }`, nil, true}, + {`{ nil !~ nil }`, nil, true}, + {`{ nil < nil }`, nil, true}, + // Duration. + {`{ 5h && 5h }`, nil, true}, + {`{ 5h || 5h }`, nil, true}, + {`{ 5h =~ 5h }`, nil, true}, + {`{ 5h !~ 5h }`, nil, true}, + // Status. + {`{ ok && ok }`, nil, true}, + {`{ ok || ok }`, nil, true}, + {`{ ok ^ ok }`, nil, true}, + {`{ ok =~ ok }`, nil, true}, + {`{ ok !~ ok }`, nil, true}, + {`{ ok <= ok }`, nil, true}, + // Kind. + {`{ client && client }`, nil, true}, + {`{ client || client }`, nil, true}, + {`{ client * client }`, nil, true}, + {`{ client =~ client }`, nil, true}, + {`{ client !~ client }`, nil, true}, + {`{ client >= client }`, nil, true}, +} + +func TestTypeCheck(t *testing.T) { + for i, tt := range typeTests { + tt := tt + t.Run(fmt.Sprintf("Test%d", i+1), func(t *testing.T) { + defer func() { + if t.Failed() { + t.Logf("Input:\n%s", tt.input) + } + }() + + got, err := Parse(tt.input) + if err != nil { + t.Logf("Error: \n%+v", err) + } + if tt.wantErr { + var se *TypeError + require.ErrorAs(t, err, &se) + return + } + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } +}