add on and ignoring clauses in binOpExpr #4391

garrettlish · 2021-09-28T12:03:01Z

What this PR does / why we need it:
Add on and ignoring clauses in binOpExpr, the on and ignoring modifier allows binary operation against different labelset.

Which issue(s) this PR fixes:
Fixes TODO TODO(owen-d): add (on,ignoring) clauses to binOpExpr

Special notes for your reviewer:

Checklist

Documentation added
Tests updated

CLAassistant · 2021-09-28T12:03:05Z

All committers have signed the CLA.

garrettlish · 2021-09-29T04:19:38Z

@owen-d this PR is target to resolve the TODO item - TODO(owen-d): add (on,ignoring) clauses to binOpExpr, it could be start of the left/right join support in LogQL as well. Any thoughts, please advise. Thanks!

cyriltovena · 2021-09-29T06:55:20Z

@owen-d let's make sure querysharding is happy with this.

cyriltovena · 2021-09-29T06:58:46Z

pkg/logql/shardmapper_test.go

@@ -675,7 +675,11 @@ func TestMapping(t *testing.T) {
 		{
 			in: `1 + sum by (cluster) (rate({foo="bar"}[5m]))`,
 			expr: &BinOpExpr{
-				op:         OpTypeAdd,
+				op: OpTypeAdd,


We need example with this new operator. The binary operation should never be shardable so I think we're fine.

thanks @cyriltovena for your comments, make sense, added example for new operator in shardmapper unit test - 9cb1a81

It's currrently implemented like this:

func (e *BinOpExpr) Shardable() bool { return shardableOps[e.op] && e.SampleExpr.Shardable() && e.RHS.Shardable() }

However, I think we should prohibit sharding when we're changing the label groupings such as in on or without as it effectively changes the labels and thus invalidates correctness guarantees in sharded aggregation. @garrettlish can you make the change to the Shardable method to return false if either on or without are specified?

pkg/logql/ast.go

cyriltovena

From my perspective the addition looks good. It would be great to have a doc addition though.

I let @owen-d give the final word.

owen-d · 2021-09-29T12:02:54Z

Awesome, I'll give this a review soon :)

owen-d

Hey @garrettlish, this looks great. Thanks for taking care of my todo for me 🎉 . I've left a few comments I'd like addressed, but then I'm happy to merge it. Really great job uncovering the complexity in here and navigating it well. I expect after this change, we'll need to implement group_{left,right} soon 😁 .

owen-d · 2021-09-29T13:26:28Z

pkg/logql/evaluator.go

 				pair := pairs[hash]
+				if pair[i] != nil {
+					failStepEvaluator(eval, errors.New("multiple matches for labels"))


Nice, was looking for this. It would be great to include a test case that hits this condition. However, instead of trying to mutate the underlying Evaluator by type switching, can we instead use a scoped error that the is set here and checked in the func() error part of the newStepEvaluator (https://github.com/grafana/loki/blob/main/pkg/logql/evaluator.go#L604)?

owen-d · 2021-09-29T13:52:56Z

pkg/logql/evaluator.go

@@ -1017,3 +1033,17 @@ func absentLabels(expr SampleExpr) labels.Labels {
 	}
 	return m
 }
+
+// failStepEvaluator marks the step evaluator as failed
+func failStepEvaluator(se StepEvaluator, err error) {


I'd prefer not to mutate via type switching here, both because mutation can make for a trickier codebase and because type switching is hard to extend well (for instance, if we add newer StepEvaluator implementations).

owen-d · 2021-09-29T14:02:30Z

pkg/logql/expr.y

@@ -33,7 +33,8 @@ import (
  str                     string
  duration                time.Duration
  LiteralExpr             *LiteralExpr
-  BinOpModifier           BinOpOptions
+  BinOpModifier           *BinOpOptions
+  BoolModifier		  *BinOpOptions


nit: formatting :)

owen-d · 2021-09-29T14:04:50Z

pkg/logql/expr.y

+	| boolModifier ON OPEN_PARENTHESIS CLOSE_PARENTHESIS
+		{
+		$$ = $1
+		$$.VectorMatching = &VectorMatching{On: true, Include: []string{}}


nit: I suspect this can be nil here

Suggested change

$$.VectorMatching = &VectorMatching{On: true, Include: []string{}}

$$.VectorMatching = &VectorMatching{On: true, Include: nil}

owen-d · 2021-09-29T14:05:18Z

pkg/logql/expr.y

+	| boolModifier IGNORING OPEN_PARENTHESIS CLOSE_PARENTHESIS
+		{
+		$$ = $1
+		$$.VectorMatching = &VectorMatching{On: false, Include: []string{}}


nit: I suspect this can be nil here

Suggested change

$$.VectorMatching = &VectorMatching{On: false, Include: []string{}}

$$.VectorMatching = &VectorMatching{On: false, Include: nil}

owen-d · 2021-09-29T14:18:33Z

pkg/logql/shardmapper_test.go

@@ -675,7 +675,11 @@ func TestMapping(t *testing.T) {
 		{
 			in: `1 + sum by (cluster) (rate({foo="bar"}[5m]))`,
 			expr: &BinOpExpr{
-				op:         OpTypeAdd,
+				op: OpTypeAdd,


It's currrently implemented like this:

func (e *BinOpExpr) Shardable() bool { return shardableOps[e.op] && e.SampleExpr.Shardable() && e.RHS.Shardable() }

However, I think we should prohibit sharding when we're changing the label groupings such as in on or without as it effectively changes the labels and thus invalidates correctness guarantees in sharded aggregation. @garrettlish can you make the change to the Shardable method to return false if either on or without are specified?

garrettlish · 2021-09-30T02:42:53Z

@owen-d thanks for your good comments, resolved all of the comments via f36957c

owen-d

Looks great! One little request then I think we're ready to merge :)

Thanks for the great PR!

owen-d · 2021-09-30T15:49:03Z

pkg/logql/ast.go

@@ -998,6 +998,10 @@ func (e *BinOpExpr) String() string {

 // impl SampleExpr
 func (e *BinOpExpr) Shardable() bool {
+	if e.opts != nil && e.opts.VectorMatching != nil {


Nit: It'd be great to have a sharding test to ensure this

Suggested change

if e.opts != nil && e.opts.VectorMatching != nil {

// `without ()` is the zero value for VectorMatching and is also equivalent

// to not specifying a VectorMatching at all and thus can

// technically pass to the next expression for sharding concerns.

var zero VectorMatching

if e.opts != nil && e.opts.VectorMatching != nil && *e.opts.VectorMatching != zero {

@owen-d
We cannot use *e.opts.VectorMatching != zero to check whether VectorMatching struct is empty, it is because VectorMatching has Include []string, it will cause Invalid operation: *e.opts.VectorMatching != zero (the operator != is not defined on VectorMatching).
I thought e.opts != nil && e.opts.VectorMatching != nil should be good enough since e.opts.VectorMatching is a pointer? Am I missing anything? Any thoughts, please advise! Thanks!

type BinOpOptions struct { ReturnBool bool VectorMatching *VectorMatching }

added sharding test - 0ab8264 and https://github.com/garrettlish/loki/blob/onOrIgnoring/pkg/logql/shardmapper_test.go#L984, could you please help review it again? Thanks!

Ah bummer, slice comparison 😭 . Anyway, e.opts != nil && e.opts.VectorMatching != nil should be fine. I was mainly protecting us from preventing sharding when the expression used without (), which is the same as not specifying without at all. I don't think that should block this PR though. Great work, LGTM!

KMiller-Grafana

I can't get the git suggestion to take my entire suggested reorg of just the docs part of this PR. It wants to reformat some of the suggestion as comment (instead of suggestion).

I'd like you to implement this rewrite. I'd like you to then further add a sentence or two just above each of the examples describing what the example demonstrates. Without that, doc readers really only see an example of the syntax.

KMiller-Grafana · 2021-09-30T21:23:58Z

My rewrite of the docs addition for file docs/sources/logql/_index.md:

Keywords on and ignoring

The ignoring keyword causes specified labels to be ignored during matching.
The syntax:

<vector expr> <bin-op> ignoring(<labels>) <vector expr>

Example:

sum by(machine) (count_over_time({app="foo"}[1m])) > bool ignoring(machine) sum(count_over_time({app="bar"}[1m]))

The on keyword reduces the set of considered labels to a specified list.
The syntax:

<vector expr> <bin-op> on(<labels>) <vector expr>

Example:

sum by(app,machine) (count_over_time({app="foo"}[1m])) + on(app) sum by (app) (count_over_time({app="bar"}[1m]))

KMiller-Grafana · 2021-09-30T21:26:37Z

Sorry @garrettlish, github formatted my rewrite. I wanted to give you the source to use.

garrettlish · 2021-10-01T15:38:48Z

@KMiller-Grafana thanks for your comments, reformat the doc based on your suggestion - 0ab8264, please help review it again, thanks!

KMiller-Grafana

Thanks for the doc rewrite. One more item is needed. Please add a sentence or 2 just before each example, describing what the example does. Without that, an example becomes a syntax example. A sentence or two will help docs readers to understand the larger picture of what the example shows, and why these keywords are useful.

garrettlish · 2021-10-02T00:17:10Z

@KMiller-Grafana sorry, I missed your first comment 😭, added description for each example: b837d8e

* use a scoped error to replace mutate stepEvaluator via type switching * change VectorMatching Include from empty string array to nil * prohibit sharding when we're changing the label groupings such as on or ignoring * add unit test to cover multiple matches for labels error * fix expr.y format issue

* reformat on and ignoring keywords doc * add sharding unit test for on and ignoring keywords

…s to understand the larger picture

will amend docs in a second pass

garrettlish requested a review from a team as a code owner September 28, 2021 12:03

pull-request-size bot added the size/XL label Sep 28, 2021

garrettlish force-pushed the onOrIgnoring branch from 7b946ac to 06a3ae7 Compare September 29, 2021 02:48

cyriltovena reviewed Sep 29, 2021

View reviewed changes

pkg/logql/ast.go Show resolved Hide resolved

cyriltovena approved these changes Sep 29, 2021

View reviewed changes

garrettlish requested a review from KMiller-Grafana as a code owner September 29, 2021 08:30

owen-d reviewed Sep 29, 2021

View reviewed changes

pull-request-size bot added size/XXL and removed size/XL labels Sep 30, 2021

garrettlish force-pushed the onOrIgnoring branch from f36957c to 581e4f2 Compare September 30, 2021 03:28

owen-d reviewed Sep 30, 2021

View reviewed changes

KMiller-Grafana suggested changes Sep 30, 2021

View reviewed changes

garrettlish force-pushed the onOrIgnoring branch from 0ab8264 to 867869f Compare October 1, 2021 16:29

KMiller-Grafana previously requested changes Oct 1, 2021

View reviewed changes

owen-d approved these changes Oct 1, 2021

View reviewed changes

garrettlish force-pushed the onOrIgnoring branch from 191dc6b to b837d8e Compare October 2, 2021 00:09

garrettlish and others added 4 commits October 2, 2021 08:24

add on and ignoring clauses in binOpExpr

11573cc

add on/ignoring new operator shardmapper unit test

0035ee3

add doc for on and ignoring keywords

73bcb19

garrettlish added 2 commits October 2, 2021 08:24

resolve comments

48314f3

* reformat on and ignoring keywords doc * add sharding unit test for on and ignoring keywords

resolve comments: add a sentence for each example to help docs reader…

8064e3d

…s to understand the larger picture

garrettlish force-pushed the onOrIgnoring branch from b837d8e to 8064e3d Compare October 2, 2021 00:52

owen-d mentioned this pull request Oct 4, 2021

Add group_{left,right} to LogQL #4407

Closed

owen-d merged commit 4417340 into grafana:main Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add on and ignoring clauses in binOpExpr #4391

add on and ignoring clauses in binOpExpr #4391

garrettlish commented Sep 28, 2021 •

edited

Loading

CLAassistant commented Sep 28, 2021 •

edited

Loading

garrettlish commented Sep 29, 2021

cyriltovena commented Sep 29, 2021

cyriltovena Sep 29, 2021

garrettlish Sep 29, 2021

owen-d Sep 29, 2021

cyriltovena left a comment •

edited

Loading

owen-d commented Sep 29, 2021

owen-d left a comment

owen-d Sep 29, 2021

owen-d Sep 29, 2021

owen-d Sep 29, 2021

owen-d Sep 29, 2021

owen-d Sep 29, 2021

owen-d Sep 29, 2021

garrettlish commented Sep 30, 2021

owen-d left a comment

owen-d Sep 30, 2021

garrettlish Oct 1, 2021 •

edited

Loading

garrettlish Oct 1, 2021

owen-d Oct 1, 2021 •

edited

Loading

KMiller-Grafana left a comment •

edited

Loading

KMiller-Grafana commented Sep 30, 2021

KMiller-Grafana commented Sep 30, 2021

garrettlish commented Oct 1, 2021

KMiller-Grafana left a comment

garrettlish commented Oct 2, 2021

	$$.VectorMatching = &VectorMatching{On: true, Include: []string{}}
	$$.VectorMatching = &VectorMatching{On: true, Include: nil}

-	if e.opts != nil && e.opts.VectorMatching != nil {
+	// `without ()` is the zero value for VectorMatching and is also equivalent
+	// to not specifying a VectorMatching at all and thus can
+	// technically pass to the next expression for sharding concerns.
+	var zero VectorMatching
+	if e.opts != nil && e.opts.VectorMatching != nil && *e.opts.VectorMatching != zero {

add on and ignoring clauses in binOpExpr #4391

add on and ignoring clauses in binOpExpr #4391

Conversation

garrettlish commented Sep 28, 2021 • edited Loading

CLAassistant commented Sep 28, 2021 • edited Loading

garrettlish commented Sep 29, 2021

cyriltovena commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyriltovena left a comment • edited Loading

Choose a reason for hiding this comment

owen-d commented Sep 29, 2021

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettlish commented Sep 30, 2021

owen-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrettlish Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owen-d Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

KMiller-Grafana left a comment • edited Loading

Choose a reason for hiding this comment

KMiller-Grafana commented Sep 30, 2021

Keywords on and ignoring

KMiller-Grafana commented Sep 30, 2021

garrettlish commented Oct 1, 2021

KMiller-Grafana left a comment

Choose a reason for hiding this comment

garrettlish commented Oct 2, 2021

garrettlish commented Sep 28, 2021 •

edited

Loading

CLAassistant commented Sep 28, 2021 •

edited

Loading

cyriltovena left a comment •

edited

Loading

garrettlish Oct 1, 2021 •

edited

Loading

owen-d Oct 1, 2021 •

edited

Loading

KMiller-Grafana left a comment •

edited

Loading