Fix parsing of vector expression (#8448)

Signed-off-by: Christian Haudum <[email protected]>
(cherry picked from commit 96d5227)

Author: chaudum

CHANGELOG.md

@@ -8,6 +8,17 @@
 
 ##### Fixes
 
+* [7784](https://github.com/grafana/loki/pull/7784) **isodude**: Fix default values of connect addresses for compactor and querier workers to work with IPv6.
+* [7880](https://github.com/grafana/loki/pull/7880) **sandeepsukhani**: consider range and offset in queries while looking for schema config for query sharding.
+* [7937](https://github.com/grafana/loki/pull/7937) **ssncferreira**: Deprecate CLI flag `-ruler.wal-cleaer.period` and replace it with `-ruler.wal-cleaner.period`.
+* [7966](https://github.com/grafana/loki/pull/7966) **sandeepsukhani**: Fix query-frontend request load balancing when using k8s service.
+* [7988](https://github.com/grafana/loki/pull/7988) **ashwanthgoli** store: write overlapping chunks to multiple stores.
+* [7925](https://github.com/grafana/loki/pull/7925) **sandeepsukhani**: Fix bugs in logs results caching causing query-frontend to return logs outside of query window.
+* [8120](https://github.com/grafana/loki/pull/8120) **ashwanthgoli** fix panic on hitting /scheduler/ring when ring is disabled.
+* [8251](https://github.com/grafana/loki/pull/8251) **sandeepsukhani** index-store: fix indexing of chunks overlapping multiple schemas.
+* [8151](https://github.com/grafana/loki/pull/8151) **sandeepsukhani** fix log deletion with line filters.
+* [8448](https://github.com/grafana/loki/pull/8448) **chaudum**: Fix bug in LogQL parser that caused certain queries that contain a vector expression to fail.
+
 ##### Changes
 
 #### Promtail

pkg/logql/syntax/ast.go

@@ -1621,7 +1621,7 @@ func (e *VectorExpr) Value() (float64, error) {
 
 func (e *VectorExpr) Selector() LogSelectorExpr               { return e }
 func (e *VectorExpr) HasFilter() bool                         { return false }
-func (e *VectorExpr) Shardable() bool                         { return true }
+func (e *VectorExpr) Shardable() bool                         { return false }
 func (e *VectorExpr) Walk(f WalkFn)                           { f(e) }
 func (e *VectorExpr) Pipeline() (log.Pipeline, error)         { return log.NewNoopPipeline(), nil }
 func (e *VectorExpr) Matchers() []*labels.Matcher             { return nil }

pkg/logql/syntax/parser.go

@@ -65,7 +65,7 @@ func (p *parser) Parse() (Expr, error) {
 
 // ParseExpr parses a string and returns an Expr.
 func ParseExpr(input string) (Expr, error) {
-	expr, err := parseExprWithoutValidation(input)
+	expr, err := ParseExprWithoutValidation(input)
 	if err != nil {
 		return nil, err
 	}
@@ -75,7 +75,7 @@ func ParseExpr(input string) (Expr, error) {
 	return expr, nil
 }
 
-func parseExprWithoutValidation(input string) (expr Expr, err error) {
+func ParseExprWithoutValidation(input string) (expr Expr, err error) {
 	if len(input) >= maxInputSize {
 		return nil, logqlmodel.NewParseError(fmt.Sprintf("input size too long (%d > %d)", len(input), maxInputSize), 0, 0)
 	}
@@ -103,31 +103,20 @@ func parseExprWithoutValidation(input string) (expr Expr, err error) {
 func validateExpr(expr Expr) error {
 	switch e := expr.(type) {
 	case SampleExpr:
-		err := validateSampleExpr(e)
-		if err != nil {
-			return err
-		}
+		return validateSampleExpr(e)
 	case LogSelectorExpr:
-		err := validateMatchers(e.Matchers())
-		if err != nil {
-			return err
-		}
+		return validateLogSelectorExpression(e)
 	default:
 		return logqlmodel.NewParseError(fmt.Sprintf("unexpected expression type: %v", e), 0, 0)
 	}
-	return nil
 }
 
 // validateMatchers checks whether a query would touch all the streams in the query range or uses at least one matcher to select specific streams.
 func validateMatchers(matchers []*labels.Matcher) error {
-	if len(matchers) == 0 {
-		return nil
-	}
 	_, matchers = util.SplitFiltersAndMatchers(matchers)
 	if len(matchers) == 0 {
 		return logqlmodel.NewParseError(errAtleastOneEqualityMatcherRequired, 0, 0)
 	}
-
 	return nil
 }
 
@@ -166,21 +155,63 @@ func ParseSampleExpr(input string) (SampleExpr, error) {
 func validateSampleExpr(expr SampleExpr) error {
 	switch e := expr.(type) {
 	case *BinOpExpr:
+		if e.err != nil {
+			return e.err
+		}
 		if err := validateSampleExpr(e.SampleExpr); err != nil {
 			return err
 		}
-
 		return validateSampleExpr(e.RHS)
-	case *LiteralExpr, *VectorExpr:
+	case *LiteralExpr:
+		if e.err != nil {
+			return e.err
+		}
+		return nil
+	case *VectorExpr:
+		if e.err != nil {
+			return e.err
+		}
+		return nil
+	case *VectorAggregationExpr:
+		if e.err != nil {
+			return e.err
+		}
+		if e.Operation == OpTypeSort || e.Operation == OpTypeSortDesc {
+			if err := validateSortGrouping(e.Grouping); err != nil {
+				return err
+			}
+		}
+		return validateSampleExpr(e.Left)
+	default:
+		selector, err := e.Selector()
+		if err != nil {
+			return err
+		}
+		return validateLogSelectorExpression(selector)
+	}
+}
+
+func validateLogSelectorExpression(expr LogSelectorExpr) error {
+	switch e := expr.(type) {
+	case *VectorExpr:
 		return nil
 	default:
-		return validateMatchers(expr.Selector().Matchers())
+		return validateMatchers(e.Matchers())
 	}
 }
 
+// validateSortGrouping prevent by|without groupings on sort operations.
+// This will keep compatibility with promql and allowing sort by (foo) doesn't make much sense anyway when sort orders by value instead of labels.
+func validateSortGrouping(grouping *Grouping) error {
+	if grouping != nil && len(grouping.Groups) > 0 {
+		return logqlmodel.NewParseError("sort and sort_desc doesn't allow grouping by ", 0, 0)
+	}
+	return nil
+}
+
 // ParseLogSelector parses a log selector expression `{app="foo"} |= "filter"`
 func ParseLogSelector(input string, validate bool) (LogSelectorExpr, error) {
-	expr, err := parseExprWithoutValidation(input)
+	expr, err := ParseExprWithoutValidation(input)
 	if err != nil {
 		return nil, err
 	}
@@ -202,7 +233,18 @@ func ParseLabels(lbs string) (labels.Labels, error) {
 	if err != nil {
 		return nil, err
 	}
+	// Sort labels to ensure functionally equivalent
+	// inputs map to the same output
 	sort.Sort(ls)
 
+	// Use the label builder to trim empty label values.
+	// Empty label values are equivalent to absent labels
+	// in Prometheus, but they unfortunately alter the
+	// Hash values created. This can cause problems in Loki
+	// if we can't rely on a set of labels to have a deterministic
+	// hash value.
+	// Therefore we must normalize early in the write path.
+	// See https://github.com/grafana/loki/pull/7355
+	// for more information
 	return labels.NewBuilder(ls).Labels(nil), nil
 }

pkg/logql/syntax/parser_test.go

@@ -2858,6 +2858,23 @@ func TestParse(t *testing.T) {
 			in:  `vector(abc)`,
 			err: logqlmodel.NewParseError("syntax error: unexpected IDENTIFIER, expecting NUMBER", 1, 8),
 		},
+		{
+			in:  `vector(1)`,
+			exp: &VectorExpr{Val: 1, err: nil},
+		},
+		{
+			in:  `label_replace(vector(0), "foo", "bar", "", "")`,
+			exp: mustNewLabelReplaceExpr(&VectorExpr{Val: 0, err: nil}, "foo", "bar", "", ""),
+		},
+		{
+			in: `sum(vector(0))`,
+			exp: &VectorAggregationExpr{
+				Left:      &VectorExpr{Val: 0, err: nil},
+				Grouping:  &Grouping{},
+				Params:    0,
+				Operation: "sum",
+			},
+		},
 		{
 			in: `{app="foo"}
 					# |= "bar"

pkg/querier/queryrange/querysharding.go

@@ -119,9 +119,6 @@ func (ast *astMapperware) Do(ctx context.Context, r queryrangebase.Request) (que
 	}
 
 	mapper := logql.NewShardMapper(resolver, ast.metrics)
-	if err != nil {
-		return nil, err
-	}
 
 	noop, parsed, err := mapper.Parse(r.GetQuery())
 	if err != nil {