fix: use syntax.MustClone to avoid panics when cloning expressions (#11595)

The clone function in rangemapper has been a persistent source of panics
due to the way stringifying exprs changed the way the original query was
represented. This PR replaces the `clone` function with
`syntax.MustClone`. It clones the expression by actually walking the AST
rather than doing `expr -> string -> parse`. It might still panic but
for a more fundamental reason than because of issues with the string
representation.
pull/9831/head^2
Travis Patterson 1 year ago committed by GitHub
parent efdea22db4
commit 3373cc54f6
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 18
      pkg/logql/rangemapper.go
  2. 13
      pkg/logql/rangemapper_test.go

@ -120,7 +120,7 @@ func (m RangeMapper) Parse(expr syntax.Expr) (bool, syntax.Expr, error) {
// is pushed down to the downstream expression.
func (m RangeMapper) Map(expr syntax.SampleExpr, vectorAggrPushdown *syntax.VectorAggregationExpr, recorder *downstreamRecorder) (syntax.SampleExpr, error) {
// immediately clone the passed expr to avoid mutating the original
expr = clone(expr)
expr = syntax.MustClone(expr)
switch e := expr.(type) {
case *syntax.VectorAggregationExpr:
return m.mapVectorAggregationExpr(e, recorder)
@ -277,7 +277,7 @@ func (m RangeMapper) vectorAggrWithRangeDownstreams(expr *syntax.RangeAggregatio
// appendDownstream adds expression expr with a range interval 'interval' and offset 'offset' to the downstreams list.
// Returns the updated downstream ConcatSampleExpr.
func appendDownstream(downstreams *ConcatSampleExpr, expr syntax.SampleExpr, interval time.Duration, offset time.Duration) *ConcatSampleExpr {
sampleExpr := clone(expr)
sampleExpr := syntax.MustClone(expr)
sampleExpr.Walk(func(e syntax.Expr) {
switch concrete := e.(type) {
case *syntax.RangeAggregationExpr:
@ -491,17 +491,3 @@ func isSplittableByRange(expr syntax.SampleExpr) bool {
return false
}
}
// clone returns a copy of the given sample expression
// This is needed whenever we want to modify the existing query tree.
// clone is identical to syntax.Expr.Clone() but with the additional type
// casting for syntax.SampleExpr.
func clone(expr syntax.SampleExpr) syntax.SampleExpr {
e, err := syntax.ParseSampleExpr(expr.String())
if err != nil {
panic(
errors.Wrapf(err, "error cloning query: %s", expr.String()),
)
}
return e
}

File diff suppressed because one or more lines are too long
Loading…
Cancel
Save