From 1c6136f023436b5740ba3c6256c6c22f3a26f23f Mon Sep 17 00:00:00 2001 From: Patryk Kwiatek Date: Thu, 18 May 2023 03:40:48 +0200 Subject: [PATCH] Aggregation expression refactor (#2644) --- .../common/aggregations}/expression.go | 101 +++++++++--------- .../expressionerrorcode_string.go | 12 +-- .../common/aggregations/operators/sum.go | 5 +- .../common/aggregations/stages/group.go | 24 +++-- .../common/aggregations/stages/unwind.go | 24 ++--- 5 files changed, 83 insertions(+), 83 deletions(-) rename internal/{types => handlers/common/aggregations}/expression.go (67%) rename internal/{types => handlers/common/aggregations}/expressionerrorcode_string.go (68%) diff --git a/internal/types/expression.go b/internal/handlers/common/aggregations/expression.go similarity index 67% rename from internal/types/expression.go rename to internal/handlers/common/aggregations/expression.go index 39bb54e3299b..eedad104b45e 100644 --- a/internal/types/expression.go +++ b/internal/handlers/common/aggregations/expression.go @@ -12,31 +12,32 @@ // See the License for the specific language governing permissions and // limitations under the License. -package types +package aggregations import ( "strings" + "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/lazyerrors" "github.com/FerretDB/FerretDB/internal/util/must" ) -//go:generate ../../bin/stringer -linecomment -type ExpressionErrorCode +//go:generate ../../../../bin/stringer -linecomment -type ExpressionErrorCode -// ExpressionErrorCode represents FieldPath error code. +// ExpressionErrorCode represents Expression error code. type ExpressionErrorCode int const ( _ ExpressionErrorCode = iota - // ErrNotFieldPath indicates that field is not a path. - ErrNotFieldPath + // ErrNotExpression indicates that field is not an expression. + ErrNotExpression - // ErrEmptyFieldPath indicates that path is empty. - ErrEmptyFieldPath + // ErrInvalidExpression indicates that expression is invalid. + ErrInvalidExpression - // ErrInvalidFieldPath indicates that path is invalid. - ErrInvalidFieldPath + // ErrEmptyFieldPath indicates that field path expression is empty. + ErrEmptyFieldPath // ErrUndefinedVariable indicates that variable name is not defined. ErrUndefinedVariable @@ -45,36 +46,30 @@ const ( ErrEmptyVariable ) -// FieldPathError describes an error that occurs getting path from field. -type FieldPathError struct { +// ExpressionError describes an error that occurs while evaluating expression. +type ExpressionError struct { code ExpressionErrorCode } -// newFieldPathError creates a new FieldPathError. -func newFieldPathError(code ExpressionErrorCode) error { - return &FieldPathError{code: code} +// newExpressionError creates a new ExpressionError. +func newExpressionError(code ExpressionErrorCode) error { + return &ExpressionError{code: code} } // Error implements the error interface. -func (e *FieldPathError) Error() string { +func (e *ExpressionError) Error() string { return e.code.String() } -// Code returns the FieldPathError code. -func (e *FieldPathError) Code() ExpressionErrorCode { +// Code returns the ExpressionError code. +func (e *ExpressionError) Code() ExpressionErrorCode { return e.code } // Expression is an expression constructed from field value. -type Expression interface { - Evaluate(doc *Document) any - GetExpressionSuffix() string -} - -// pathExpression is field path constructed from expression. -type pathExpression struct { - path Path +type Expression struct { *ExpressionOpts + path types.Path } // ExpressionOpts represents options used to modify behavior of Expression functions. @@ -88,7 +83,7 @@ type ExpressionOpts struct { // NewExpressionWithOpts creates a new instance by checking expression string. // It can take additional opts that specify how expressions should be evaluated. -func NewExpressionWithOpts(expression string, opts *ExpressionOpts) (Expression, error) { +func NewExpressionWithOpts(expression string, opts *ExpressionOpts) (*Expression, error) { // TODO https://github.com/FerretDB/FerretDB/issues/2348 var val string @@ -97,54 +92,55 @@ func NewExpressionWithOpts(expression string, opts *ExpressionOpts) (Expression, // `$$` indicates field is a variable. v := strings.TrimPrefix(expression, "$$") if v == "" { - return nil, newFieldPathError(ErrEmptyVariable) + return nil, newExpressionError(ErrEmptyVariable) } if strings.HasPrefix(v, "$") { - return nil, newFieldPathError(ErrInvalidFieldPath) + return nil, newExpressionError(ErrInvalidExpression) } // TODO https://github.com/FerretDB/FerretDB/issues/2275 - return nil, newFieldPathError(ErrUndefinedVariable) + return nil, newExpressionError(ErrUndefinedVariable) case strings.HasPrefix(expression, "$"): // `$` indicates field is a path. val = strings.TrimPrefix(expression, "$") if val == "" { - return nil, newFieldPathError(ErrEmptyFieldPath) + return nil, newExpressionError(ErrEmptyFieldPath) } default: - return nil, newFieldPathError(ErrNotFieldPath) + return nil, newExpressionError(ErrNotExpression) } var err error - path, err := NewPathFromString(val) + path, err := types.NewPathFromString(val) if err != nil { return nil, lazyerrors.Error(err) } - return &pathExpression{ + return &Expression{ path: path, ExpressionOpts: opts, }, nil } // NewExpression creates a new instance by checking expression string. -func NewExpression(expression string) (Expression, error) { +func NewExpression(expression string) (*Expression, error) { // TODO https://github.com/FerretDB/FerretDB/issues/2348 return NewExpressionWithOpts(expression, new(ExpressionOpts)) } // Evaluate gets the value at the path. -func (p *pathExpression) Evaluate(doc *Document) any { - path := p.path +// It returns `types.Null` if the path does not exists. +func (e *Expression) Evaluate(doc *types.Document) any { + path := e.path if path.Len() == 1 { val, err := doc.Get(path.String()) if err != nil { - // if the path does not exist, return nil. - return Null + // $group stage groups non-existent paths with `Null` + return types.Null } return val @@ -154,20 +150,21 @@ func (p *pathExpression) Evaluate(doc *Document) any { prefix := path.Prefix() if v, err := doc.Get(prefix); err == nil { - if _, isArray := v.(*Array); isArray { + if _, isArray := v.(*types.Array); isArray { isPrefixArray = true } } - vals := p.getExpressionPathValue(doc, path) + vals := e.getPathValue(doc, path) if len(vals) == 0 { if isPrefixArray { // when the prefix is array, return empty array. - return must.NotFail(NewArray()) + return must.NotFail(types.NewArray()) } - return Null + // $group stage groups non-existent paths with `Null` + return types.Null } if len(vals) == 1 && !isPrefixArray { @@ -176,7 +173,7 @@ func (p *pathExpression) Evaluate(doc *Document) any { } // when the prefix is array, return an array of value. - arr := MakeArray(len(vals)) + arr := types.MakeArray(len(vals)) for _, v := range vals { arr.Append(v) } @@ -185,11 +182,11 @@ func (p *pathExpression) Evaluate(doc *Document) any { } // GetExpressionSuffix returns suffix of pathExpression. -func (p *pathExpression) GetExpressionSuffix() string { - return p.path.Suffix() +func (e *Expression) GetExpressionSuffix() string { + return e.path.Suffix() } -// getExpressionPathValue go through each key of the path iteratively to +// getPathValue go through each key of the path iteratively to // find values that exist at suffix. // An array may return multiple values. // At each key of the path, it checks: @@ -197,10 +194,10 @@ func (p *pathExpression) GetExpressionSuffix() string { // - if the array contains documents which have the key. (This check can // be disabled by setting ExpressionOpts.IgnoreArrays field). // -// It is different from `getDocumentsAtSuffix`, it does not find array item by +// It is different from `common.getDocumentsAtSuffix`, it does not find array item by // array dot notation `foo.0.bar`. It returns empty array [] because using index // such as `0` does not match using expression path. -func (p *pathExpression) getExpressionPathValue(doc *Document, path Path) []any { +func (e *Expression) getPathValue(doc *types.Document, path types.Path) []any { // TODO https://github.com/FerretDB/FerretDB/issues/2348 keys := path.Slice() vals := []any{doc} @@ -211,22 +208,22 @@ func (p *pathExpression) getExpressionPathValue(doc *Document, path Path) []any for _, valAtKey := range vals { switch val := valAtKey.(type) { - case *Document: + case *types.Document: embeddedVal, err := val.Get(key) if err != nil { continue } embeddedVals = append(embeddedVals, embeddedVal) - case *Array: - if p.IgnoreArrays { + case *types.Array: + if e.IgnoreArrays { continue } // iterate elements to get documents that contain the key. for j := 0; j < val.Len(); j++ { elem := must.NotFail(val.Get(j)) - docElem, isDoc := elem.(*Document) + docElem, isDoc := elem.(*types.Document) if !isDoc { continue } diff --git a/internal/types/expressionerrorcode_string.go b/internal/handlers/common/aggregations/expressionerrorcode_string.go similarity index 68% rename from internal/types/expressionerrorcode_string.go rename to internal/handlers/common/aggregations/expressionerrorcode_string.go index 19f6beb590f5..faa081aa1d33 100644 --- a/internal/types/expressionerrorcode_string.go +++ b/internal/handlers/common/aggregations/expressionerrorcode_string.go @@ -1,6 +1,6 @@ // Code generated by "stringer -linecomment -type ExpressionErrorCode"; DO NOT EDIT. -package types +package aggregations import "strconv" @@ -8,16 +8,16 @@ func _() { // An "invalid array index" compiler error signifies that the constant values have changed. // Re-run the stringer command to generate them again. var x [1]struct{} - _ = x[ErrNotFieldPath-1] - _ = x[ErrEmptyFieldPath-2] - _ = x[ErrInvalidFieldPath-3] + _ = x[ErrNotExpression-1] + _ = x[ErrInvalidExpression-2] + _ = x[ErrEmptyFieldPath-3] _ = x[ErrUndefinedVariable-4] _ = x[ErrEmptyVariable-5] } -const _ExpressionErrorCode_name = "ErrNotFieldPathErrEmptyFieldPathErrInvalidFieldPathErrUndefinedVariableErrEmptyVariable" +const _ExpressionErrorCode_name = "ErrNotExpressionErrInvalidExpressionErrEmptyFieldPathErrUndefinedVariableErrEmptyVariable" -var _ExpressionErrorCode_index = [...]uint8{0, 15, 32, 51, 71, 87} +var _ExpressionErrorCode_index = [...]uint8{0, 16, 36, 53, 73, 89} func (i ExpressionErrorCode) String() string { i -= 1 diff --git a/internal/handlers/common/aggregations/operators/sum.go b/internal/handlers/common/aggregations/operators/sum.go index 9fda68dc8912..ebc696757306 100644 --- a/internal/handlers/common/aggregations/operators/sum.go +++ b/internal/handlers/common/aggregations/operators/sum.go @@ -19,6 +19,7 @@ import ( "math" "math/big" + "github.com/FerretDB/FerretDB/internal/handlers/common/aggregations" "github.com/FerretDB/FerretDB/internal/handlers/commonerrors" "github.com/FerretDB/FerretDB/internal/types" "github.com/FerretDB/FerretDB/internal/util/must" @@ -26,7 +27,7 @@ import ( // sum represents $sum aggregation operator. type sum struct { - expression types.Expression + expression *aggregations.Expression number any } @@ -46,7 +47,7 @@ func newSum(accumulation *types.Document) (Accumulator, error) { accumulator.number = expr case string: var err error - if accumulator.expression, err = types.NewExpression(expr); err != nil { + if accumulator.expression, err = aggregations.NewExpression(expr); err != nil { // $sum returns 0 on non-existent field. accumulator.number = int32(0) } diff --git a/internal/handlers/common/aggregations/stages/group.go b/internal/handlers/common/aggregations/stages/group.go index 478144028c43..378cee41abb0 100644 --- a/internal/handlers/common/aggregations/stages/group.go +++ b/internal/handlers/common/aggregations/stages/group.go @@ -187,46 +187,48 @@ func (g *group) groupDocuments(ctx context.Context, in []*types.Document) ([]gro }}, nil } - expression, err := types.NewExpression(groupKey) + expression, err := aggregations.NewExpression(groupKey) if err != nil { - var fieldPathErr *types.FieldPathError - if !errors.As(err, &fieldPathErr) { + var exprErr *aggregations.ExpressionError + if !errors.As(err, &exprErr) { return nil, lazyerrors.Error(err) } - switch fieldPathErr.Code() { - case types.ErrNotFieldPath: + switch exprErr.Code() { + case aggregations.ErrNotExpression: // constant value aggregates values of all `in` documents into one aggregated document. return []groupedDocuments{{ groupID: groupKey, documents: in, }}, nil - case types.ErrEmptyFieldPath: + case aggregations.ErrEmptyFieldPath: return nil, commonerrors.NewCommandErrorMsgWithArgument( + // TODO commonerrors.ErrGroupInvalidFieldPath, - "'$' by itself is not a valid FieldPath", + "'$' by itself is not a valid Expression", "$group (stage)", ) - case types.ErrInvalidFieldPath: + case aggregations.ErrInvalidExpression: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrFailedToParse, fmt.Sprintf("'%s' starts with an invalid character for a user variable name", types.FormatAnyValue(groupKey)), "$group (stage)", ) - case types.ErrEmptyVariable: + case aggregations.ErrEmptyVariable: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrFailedToParse, "empty variable names are not allowed", "$group (stage)", ) - case types.ErrUndefinedVariable: + // TODO https://github.com/FerretDB/FerretDB/issues/2275 + case aggregations.ErrUndefinedVariable: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrGroupUndefinedVariable, fmt.Sprintf("Use of undefined variable: %s", types.FormatAnyValue(groupKey)), "$group (stage)", ) default: - panic(fmt.Sprintf("unhandled field path error %s", fieldPathErr.Error())) + panic(fmt.Sprintf("unhandled field path error %s", exprErr.Error())) } } diff --git a/internal/handlers/common/aggregations/stages/unwind.go b/internal/handlers/common/aggregations/stages/unwind.go index cdfe76500138..b7c37ab6e7d5 100644 --- a/internal/handlers/common/aggregations/stages/unwind.go +++ b/internal/handlers/common/aggregations/stages/unwind.go @@ -30,7 +30,7 @@ import ( // unwind represents $unwind stage. type unwind struct { - field types.Expression + field *aggregations.Expression } // newUnwind creates a new $unwind stage. @@ -40,7 +40,7 @@ func newUnwind(stage *types.Document) (aggregations.Stage, error) { return nil, err } - var expr types.Expression + var expr *aggregations.Expression switch field := field.(type) { case *types.Document: @@ -54,34 +54,34 @@ func newUnwind(stage *types.Document) (aggregations.Stage, error) { ) } - opts := types.ExpressionOpts{ + opts := aggregations.ExpressionOpts{ IgnoreArrays: true, } - expr, err = types.NewExpressionWithOpts(field, &opts) + expr, err = aggregations.NewExpressionWithOpts(field, &opts) if err != nil { - var fieldPathErr *types.FieldPathError - if !errors.As(err, &fieldPathErr) { + var exprErr *aggregations.ExpressionError + if !errors.As(err, &exprErr) { return nil, lazyerrors.Error(err) } - switch fieldPathErr.Code() { - case types.ErrNotFieldPath: + switch exprErr.Code() { + case aggregations.ErrNotExpression: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrStageUnwindNoPrefix, fmt.Sprintf("path option to $unwind stage should be prefixed with a '$': %v", types.FormatAnyValue(field)), "$unwind (stage)", ) - case types.ErrEmptyFieldPath: + case aggregations.ErrEmptyFieldPath: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrEmptyFieldPath, - "FieldPath cannot be constructed with empty string", + "Expression cannot be constructed with empty string", "$unwind (stage)", ) - case types.ErrEmptyVariable, types.ErrInvalidFieldPath, types.ErrUndefinedVariable: + case aggregations.ErrEmptyVariable, aggregations.ErrInvalidExpression, aggregations.ErrUndefinedVariable: return nil, commonerrors.NewCommandErrorMsgWithArgument( commonerrors.ErrFieldPathInvalidName, - "FieldPath field names may not start with '$'. Consider using $getField or $setField", + "Expression field names may not start with '$'. Consider using $getField or $setField", "$unwind (stage)", ) default: