feat(engine): basic math expressions

[spiridonov]

What this PR does / why we need it:

In order to add support for rate() aggregation function, which will be implemented as count_over_time/$interval, I had to add support for math expressions. Math expressions are implemented as Projections, where Expand expression is evaluated from other columns and added to the result. Binary expressions with only single input are supported for now. Things like sum_over_time/count_over_time will be implemented later. Two inputs will be implemented as inner joins. For now I only added some plumbing to the planner for adding Join node before math expressions with two inputs. Joins themselves are not implemented yet, and the logical planner will throw a NotImplementer error if such a query is passed into it, but the plumbing code is there for better picture of overall design.

• I restricted Projections to a single expand math expression because it is unclear how to evaluate multiple expressions each of which adds value column as the result.
• I also refactored Planner.process() to return a single Node for simplicity. I do not see a case where it can return several children at once, however I see examples of "complex" children (MakeTable or Projection) where a subtree can be returned instead of a single node.

Minor:

• fixed some typos

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Diff for pkg/engine/internal/planner/logical/planner.go is ugly here, it is better to view the new file as the whole to understand it better. I basically split one large function into 3 pieces without changing much in that logic.

Checklist

• Reviewed the CONTRIBUTING.md guide (required)
• Documentation added
• Tests updated
• Title matches the required conventional commits format, see here
  • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
• Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
• If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

[rfratto]

Thanks, that makes sense if you're trying to prepare for supporting math over two vectors.

I suspect computations over two vectors (in a way that's compatible with LogQL/PromQL) is going to be trickier than it seems. I believe, in relational algebra terms, they're expressed as a combination of an inner join and a projection.

So, given a query like

(
  sum by (job) (rate({namespace="dev"}[$__auto])
) + (
  sum by (job) (rate({namespace="qa"}[$__auto])
)

A physical plan mapping literally to the algebra could be

Projection expressions=[
  left.timestamp as timestamp,
  left.job AS job, 
  left.value + right.value as value,
] 
  InnerJoin left_prefix="left" right_prefix="right" on=[
    left.timestamp = right.timestamp && left.job = right.job 
  ] 
    VectorAggregate op=sum groupings=[job] # left-hand side 
      RangeAggregate op=rate  
        DataObjScan 
    VectorAggregate op=sum groupings=[job] # right-hand side 
      RangeAggregate op=rate 
        DataObjScan 

This needs to explicitly be an inner join, since LogQL's metric queries require the sample to exist on both side of the expression (this matches PromQL's behaviour).

Example

For example, the two inputs of OuterJoin

ts	job	value
0	loki	5
10	mimir	10

and

ts	job	value
0	loki	15
10	tempo	10

is joined into

left.ts	left.job	left.value	right.ts	right.job	right.value
0	loki	5	0	loki	15

and is projected to

unix_ts	job	value
0	loki	20

I think it's probably okay if we wanted to have a node which combines the work of projection and inner joins, though I do think it's possible to represent these operations using projections, which we will have a separate node for, and separating them out may be easier to understand in the plans.

All that said, I do wonder if math on two vectors is going to require a lot more thought. Would we be able to simplify the logic here if we descoped that from our consideration?

[ashwanthgoli]

lgtm

[chaudum]

Could you also add some test cases to pkg/engine/internal/planner/planner_test.go

[chaudum]

An invalid op type should return an error.

[spiridonov]

We have an inconsistent pattern for function like convertBinaryArithmeticOp. Some panic, and some return Ivalid enum value. Here I return Invalid that indicates that this op is not implemented. I don't want to panic here, and I don't want to make the function more complex by returning error just for that.

[chaudum]

leftInput and rightInput are nil only in case they are literals, arent't they?
So checking for !- nil does not mean the node is a scan node? This assumption seems error prone.

[spiridonov]

leftInput and rightInput are nil if they are literals, bin ops or unary ops.

[chaudum]

What is the Join used for? I cannot see a usage in the executor. Should the executor return an error in case sees a join in the plan?

I assume this is the plumbing you mentioned in the PR description

Two inputs will be implemented as inner joins. For now I only added some plumbing to the planner for adding Join node before math expressions with two inputs. Joins themselves are not implemented yet, and the logical planner will throw a NotImplementer error if such a query is passed into it, but the plumbing code is there for better picture of overall design.

[spiridonov]

Yes, this is that plumbing from the description. Join is not implemented in the executor yet. But this code is not actually reachable because the logical planner will throw nonImplemented error when there are math expressions with two inputs.

[chaudum]

Could there be a problem with colliding column names if there are nested math expressions?

[spiridonov]

There might be something I am not aware of. But there are couple unit tests that test nested math expressions and they behave as expected.