Expression post-parsing: infer segment vs dimension (#14232)

Use the type checker for dynamic resolving of an identifier, i.e. to infer
whether it represents a dimension or a segment, based on the type
constraints in the sub-expression.
This also eliminates the flawed type-based resolving which happens
during the parsing time.

frontend/src/metabase/lib/expressions/compile.js

@@ -76,21 +76,22 @@ class ExpressionMBQLCompilerVisitor extends ExpressionCstVisitor {
     }
     return ["metric", metric.id];
   }
-  segmentExpression(ctx) {
-    const segmentName = this.visit(ctx.segmentName);
-    const segment = parseSegment(segmentName, this._options);
-    if (!segment) {
-      throw new Error(`Unknown Segment: ${segmentName}`);
-    }
-    return ["segment", segment.id];
-  }
   dimensionExpression(ctx) {
-    const dimensionName = this.visit(ctx.dimensionName);
-    const dimension = parseDimension(dimensionName, this._options);
-    if (!dimension) {
-      throw new Error(`Unknown Field: ${dimensionName}`);
+    const name = this.visit(ctx.dimensionName);
+    if (ctx.resolveAs === "segment") {
+      const segment = parseSegment(name, this._options);
+      if (!segment) {
+        throw new Error(`Unknown Segment: ${name}`);
+      }
+      return ["segment", segment.id];
+    } else {
+      // fallback
+      const dimension = parseDimension(name, this._options);
+      if (!dimension) {
+        throw new Error(`Unknown Field: ${name}`);
+      }
+      return dimension.mbql();
     }
-    return dimension.mbql();
   }
 
   identifier(ctx) {

frontend/src/metabase/lib/expressions/parser.js

@@ -250,13 +250,6 @@ export class ExpressionParser extends CstParser {
       ]);
     });
 
-    $.RULE("segmentExpression", () => {
-      $.OR([
-        { ALT: () => $.SUBRULE($.identifierString, { LABEL: "segmentName" }) },
-        { ALT: () => $.SUBRULE($.identifier, { LABEL: "segmentName" }) },
-      ]);
-    });
-
     $.RULE("dimensionExpression", () => {
       $.OR([
         {
@@ -326,13 +319,6 @@ export class ExpressionParser extends CstParser {
                 LABEL: "expression",
               }),
           },
-          {
-            GATE: () => isExpressionType("boolean", returnType),
-            ALT: () =>
-              $.SUBRULE($.segmentExpression, {
-                LABEL: "expression",
-              }),
-          },
           // expressions
           {
             GATE: () =>
@@ -347,7 +333,8 @@ export class ExpressionParser extends CstParser {
           {
             GATE: () =>
               isExpressionType("string", returnType) ||
-              isExpressionType("number", returnType),
+              isExpressionType("number", returnType) ||
+              isExpressionType("boolean", returnType),
             ALT: () =>
               $.SUBRULE($.dimensionExpression, {
                 LABEL: "expression",

frontend/src/metabase/lib/expressions/suggest.js

@@ -141,7 +141,7 @@ export function suggest({
         (isExpressionType(expectedType, "expression") ||
           isExpressionType(expectedType, "boolean"));
       const isSegment =
-        parentRule === "segmentExpression" &&
+        parentRule === "dimensionExpression" &&
         isExpressionType(expectedType, "boolean");
       const isMetric =
         parentRule === "metricExpression" &&
@@ -526,7 +526,6 @@ const ALL_RULES = [
   "functionExpression",
   "caseExpression",
   "metricExpression",
-  "segmentExpression",
   "dimensionExpression",
   "identifier",
   "identifierString",

frontend/src/metabase/lib/expressions/syntax.js

@@ -132,13 +132,8 @@ export class ExpressionSyntaxVisitor extends ExpressionCstVisitor {
     const metricName = this.visit(ctx.metricName);
     return syntaxNode("metric", metricName);
   }
-  segmentExpression(ctx) {
-    const segmentName = this.visit(ctx.segmentName);
-    return syntaxNode("segment", segmentName);
-  }
   dimensionExpression(ctx) {
-    const dimensionName = this.visit(ctx.dimensionName);
-    return syntaxNode("dimension", dimensionName);
+    return syntaxNode(ctx.resolveAs, this.visit(ctx.dimensionName));
   }
 
   identifier(ctx) {

frontend/src/metabase/lib/expressions/typechecker.js

@@ -45,18 +45,15 @@ export function typeCheck(cst, rootType) {
       return super.metricExpression(ctx);
     }
 
-    segmentExpression(ctx) {
-      const type = this.typeStack[0];
-      if (type !== "boolean") {
-        throw new Error("Incorrect type for segment");
-      }
-      return super.segmentExpression(ctx);
-    }
-
     dimensionExpression(ctx) {
       const type = this.typeStack[0];
-      if (type === "boolean" || type === "aggregation") {
-        throw new Error("Incorrect type for dimension");
+      if (type === "boolean") {
+        ctx.resolveAs = "segment";
+      } else {
+        ctx.resolveAs = "dimension";
+        if (type === "aggregation") {
+          throw new Error("Incorrect type for dimension");
+        }
       }
       return super.dimensionExpression(ctx);
     }

frontend/src/metabase/lib/expressions/visitor.js

@@ -49,9 +49,6 @@ export class ExpressionVisitor {
   metricExpression(ctx) {
     return this.visit(ctx.metricName);
   }
-  segmentExpression(ctx) {
-    return this.visit(ctx.segmentName);
-  }
   dimensionExpression(ctx) {
     return this.visit(ctx.dimensionName);
   }

frontend/test/metabase/lib/expressions/typechecker.unit.spec.js

@@ -41,11 +41,13 @@ describe("type-checker", () => {
       metricExpression(ctx) {
         this.metrics.push(this.visit(ctx.metricName));
       }
-      segmentExpression(ctx) {
-        this.segments.push(this.visit(ctx.segmentName));
-      }
       dimensionExpression(ctx) {
-        this.dimensions.push(this.visit(ctx.dimensionName));
+        const name = this.visit(ctx.dimensionName);
+        if (ctx.resolveAs === "segment") {
+          this.segments.push(name);
+        } else {
+          this.dimensions.push(name);
+        }
       }
     }
     const tree = parseSource(source, startRule);