Loki Query Builder: binary expression and numeric literal bugs (#74950)

* Following similar changes made to prometheus in #47198, reverse the order of binary operator parameter array, and fix bugs introduced by importing prometheus lezer constants into loki parser

* add unit test asserting buildVisualQueryFromString does not properly parse queries with nested binary operations

* fix onOrIgnoring parsing logic which was always stripping out the value of the matcher when a boolean was found

public/app/plugins/datasource/loki/querybuilder/binaryScalarOperations.ts

@@ -82,12 +82,12 @@ export const binaryScalarOperations: QueryBuilderOperationDef[] = binaryScalarDe
   const params: QueryBuilderOperationParamDef[] = [{ name: 'Value', type: 'number' }];
   const defaultParams: any[] = [2];
   if (opDef.comparison) {
-    params.unshift({
+    params.push({
       name: 'Bool',
       type: 'boolean',
       description: 'If checked comparison will return 0 or 1 for the value rather than filtering.',
     });
-    defaultParams.unshift(false);
+    defaultParams.push(false);
   }
 
   return {
@@ -107,8 +107,7 @@ function getSimpleBinaryRenderer(operator: string) {
     let param = model.params[0];
     let bool = '';
     if (model.params.length === 2) {
-      param = model.params[1];
+      bool = model.params[1] ? ' bool' : '';
-      bool = model.params[0] ? ' bool' : '';
     }
 
     return `${innerExpr} ${operator}${bool} ${param}`;

public/app/plugins/datasource/loki/querybuilder/parsing.test.ts

@@ -11,6 +11,116 @@ describe('buildVisualQueryFromString', () => {
     );
   });
 
+  it('parses simple binary comparison', () => {
+    expect(buildVisualQueryFromString('count_over_time({app="aggregator"} [$__auto]) == 11')).toEqual({
+      query: {
+        labels: [
+          {
+            label: 'app',
+            op: '=',
+            value: 'aggregator',
+          },
+        ],
+        operations: [
+          {
+            id: LokiOperationId.CountOverTime,
+            params: ['$__auto'],
+          },
+          {
+            id: LokiOperationId.EqualTo,
+            // defined in getSimpleBinaryRenderer, the first argument is the bool value, and the second is the comparison operator
+            params: [11, false],
+          },
+        ],
+      },
+      errors: [],
+    });
+  });
+
+  // This still fails because loki doesn't properly parse the bool operator
+  it('parses simple query with label-values with boolean operator', () => {
+    expect(buildVisualQueryFromString('count_over_time({app="aggregator"} [$__auto]) == bool 12')).toEqual({
+      query: {
+        labels: [
+          {
+            label: 'app',
+            op: '=',
+            value: 'aggregator',
+          },
+        ],
+        operations: [
+          {
+            id: LokiOperationId.CountOverTime,
+            params: ['$__auto'],
+          },
+          {
+            id: LokiOperationId.EqualTo,
+            // defined in getSimpleBinaryRenderer, the first argument is the bool value, and the second is the comparison operator
+            params: [12, true],
+          },
+        ],
+      },
+      errors: [],
+    });
+  });
+
+  it('parses binary operation with query', () => {
+    expect(
+      // There is no capability for "bool" in the query builder for (nested) binary operation with query as of now, it will always be stripped out
+      buildVisualQueryFromString(
+        'max by(stream) (count_over_time({app="aggregator"}[1m])) > bool ignoring(stream) avg(count_over_time({app="aggregator"}[1m]))'
+      )
+    ).toEqual({
+      query: {
+        binaryQueries: [
+          {
+            // nested binary operation
+            operator: '>',
+            query: {
+              labels: [
+                {
+                  label: 'app',
+                  op: '=',
+                  value: 'aggregator',
+                },
+              ],
+              operations: [
+                {
+                  id: 'count_over_time',
+                  params: ['1m'],
+                },
+                {
+                  id: 'avg',
+                  params: [],
+                },
+              ],
+            },
+            vectorMatches: 'stream',
+            vectorMatchesType: 'ignoring',
+          },
+        ],
+        labels: [
+          {
+            label: 'app',
+            op: '=',
+            value: 'aggregator',
+          },
+        ],
+        operations: [
+          {
+            id: 'count_over_time',
+            params: ['1m'],
+          },
+          {
+            id: '__max_by',
+            params: ['stream'],
+          },
+        ],
+      },
+      errors: [],
+    });
+  });
+
   it('parses simple query with label-values', () => {
     expect(buildVisualQueryFromString('{app="frontend"}')).toEqual(
       noErrors({

public/app/plugins/datasource/loki/querybuilder/parsing.ts

@@ -1,5 +1,4 @@
 import { SyntaxNode } from '@lezer/common';
-import { BinModifiers, OnOrIgnoring } from '@prometheus-io/lezer-promql';
 
 import {
   And,
@@ -50,6 +49,8 @@ import {
   VectorAggregationExpr,
   VectorOp,
   Without,
+  BinOpModifier,
+  OnOrIgnoringModifier,
 } from '@grafana/lezer-logql';
 
 import {
@@ -565,7 +566,7 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) {
   const visQuery = context.query;
   const left = node.firstChild!;
   const op = getString(expr, left.nextSibling);
-  const binModifier = getBinaryModifier(expr, node.getChild(BinModifiers));
+  const binModifier = getBinaryModifier(expr, node.getChild(BinOpModifier));
 
   const right = node.lastChild!;
 
@@ -591,7 +592,7 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) {
     // Due to the way binary ops are parsed we can get a binary operation on the right that starts with a number which
     // is a factor for a current binary operation. So we have to add it as an operation now.
     const leftMostChild = getLeftMostChild(right);
-    if (leftMostChild?.name === 'Number') {
+    if (leftMostChild?.type.id === NumberLezer) {
       visQuery.operations.push(makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool));
     }
 
@@ -624,15 +625,17 @@ function getBinaryModifier(
   node: SyntaxNode | null
 ):
   | { isBool: true; isMatcher: false }
-  | { isBool: false; isMatcher: true; matches: string; matchType: 'ignoring' | 'on' }
+  | { isBool: boolean; isMatcher: true; matches: string; matchType: 'ignoring' | 'on' }
   | undefined {
   if (!node) {
     return undefined;
   }
-  if (node.getChild(Bool)) {
+  const matcher = node.getChild(OnOrIgnoringModifier);
+  const boolMatcher = node.getChild(Bool);
+
+  if (!matcher && boolMatcher) {
     return { isBool: true, isMatcher: false };
   } else {
-    const matcher = node.getChild(OnOrIgnoring);
     if (!matcher) {
       // Not sure what this could be, maybe should be an error.
       return undefined;
@@ -640,7 +643,7 @@ function getBinaryModifier(
     const labels = getString(expr, matcher.getChild(GroupingLabels)?.getChild(GroupingLabelList));
     return {
       isMatcher: true,
-      isBool: false,
+      isBool: !!boolMatcher,
       matches: labels,
       matchType: matcher.getChild(On) ? 'on' : 'ignoring',
     };

public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts

@@ -1,5 +1,5 @@
 import { buildVisualQueryFromString } from './parsing';
-import { PromVisualQuery } from './types';
+import { PromOperationId, PromVisualQuery } from './types';
 
 describe('buildVisualQueryFromString', () => {
   it('creates no errors for empty query', () => {
@@ -11,6 +11,50 @@ describe('buildVisualQueryFromString', () => {
       })
     );
   });
+  it('parses simple binary comparison', () => {
+    expect(buildVisualQueryFromString('{app="aggregator"} == 11')).toEqual({
+      query: {
+        labels: [
+          {
+            label: 'app',
+            op: '=',
+            value: 'aggregator',
+          },
+        ],
+        metric: '',
+        operations: [
+          {
+            id: PromOperationId.EqualTo,
+            params: [11, false],
+          },
+        ],
+      },
+      errors: [],
+    });
+  });
+
+  // This still fails because loki doesn't properly parse the bool operator
+  it('parses simple query with with boolean operator', () => {
+    expect(buildVisualQueryFromString('{app="aggregator"} == bool 12')).toEqual({
+      query: {
+        labels: [
+          {
+            label: 'app',
+            op: '=',
+            value: 'aggregator',
+          },
+        ],
+        metric: '',
+        operations: [
+          {
+            id: PromOperationId.EqualTo,
+            params: [12, true],
+          },
+        ],
+      },
+      errors: [],
+    });
+  });
   it('parses simple query', () => {
     expect(buildVisualQueryFromString('counters_logins{app="frontend"}')).toEqual(
       noErrors({