Change grammar to enforce aggregation arguments, misc cleanup

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

@@ -70,5 +70,7 @@ function formatMath([operator, ...args], info, parens) {
 
 function formatAggregation([aggregation, ...args], info) {
     const { aggregations } = info;
-    return `${aggregations.get(aggregation)}(${args.map(arg => format(arg, info)).join(", ")})`;
+    return args.length === 0 ?
+        aggregations.get(aggregation) :
+        `${aggregations.get(aggregation)}(${args.map(arg => format(arg, info)).join(", ")})`;
 }

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

@@ -11,20 +11,22 @@ export function formatAggregationName(aggregationOption) {
     return VALID_AGGREGATIONS.get(aggregationOption.short);
 }
 
+function formatIdentifier(name) {
+    return /^\w+$/.test(name) ?
+        name :
+        JSON.stringify(name);
+}
+
 export function formatMetricName(metric) {
     return titleize(metric.name).replace(/\W+/g, "")
 }
 
 export function formatFieldName(field) {
-    return /^\w+$/.test(field.display_name) ?
-        field.display_name :
-        JSON.stringify(field.display_name);
+    return formatIdentifier(field.display_name);
 }
 
 export function formatExpressionName(name) {
-    return /^\w+$/.test(name) ?
-        name :
-        JSON.stringify(name);
+    return formatIdentifier(name);
 }
 
 // move to query lib

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

@@ -9,7 +9,7 @@ import {
     allTokens,
     LParen, RParen, Comma,
     AdditiveOperator, MultiplicativeOperator,
-    Aggregation,
+    Aggregation, NullaryAggregation, UnaryAggregation,
     StringLiteral, NumberLiteral,
     Identifier
 } from "./tokens";
@@ -59,30 +59,46 @@ class ExpressionsParser extends Parser {
             return this._math(initial, operations);
         });
 
-        $.RULE("aggregationOrMetricExpression", (outsideAggregation) => {
-            return $.OR([
-                {ALT: () => $.SUBRULE($.aggregationExpression, [outsideAggregation]) },
-                {ALT: () => $.SUBRULE($.metricExpression) }
-            ]);
-        });
+        $.RULE("nullaryCall", () => {
+            return {
+                lParen: $.CONSUME(LParen),
+                rParen: $.CONSUME(RParen)
+            }
+        })
+        $.RULE("unaryCall", () => {
+            return {
+                lParen: $.CONSUME(LParen),
+                arg:    $.SUBRULE($.expression, [false]),
+                rParen: $.CONSUME(RParen)
+            }
+        })
 
         $.RULE("aggregationExpression", (outsideAggregation) => {
-            const aggregation = $.CONSUME(Aggregation);
-            const lParen = $.CONSUME(LParen);
-            const args = $.MANY_SEP(Comma, () => $.SUBRULE($.expression, [false]));
-            const rParen = $.CONSUME(RParen);
-
-            return this._aggregation(aggregation, lParen, args, rParen);
+            const { aggregation, lParen, arg, rParen } = $.OR([
+                {ALT: () => ({
+                    aggregation: $.CONSUME(NullaryAggregation),
+                    ...$.OPTION(() => $.SUBRULE($.nullaryCall))
+                })},
+                {ALT: () => ({
+                    aggregation: $.CONSUME(UnaryAggregation),
+                    ...$.SUBRULE($.unaryCall)
+                })}
+            ]);
+            return this._aggregation(aggregation, lParen, arg, rParen);
         });
 
         $.RULE("metricExpression", () => {
             const metricName = $.SUBRULE($.identifier);
             const lParen = $.CONSUME(LParen);
             const rParen = $.CONSUME(RParen);
+            // const metricName = $.OR([
+            //     {ALT: () => $.SUBRULE($.stringLiteral) },
+            //     {ALT: () => $.SUBRULE($.identifier) }
+            // ]);
 
             const metric = this.getMetricForName(this._toString(metricName));
             if (metric != null) {
-                return this._metricReference(metricName, metric.id);
+                return this._metricReference(metricName, lParen, rParen, metric.id);
             }
             return this._unknownMetric(metricName, lParen, rParen);
         });
@@ -121,13 +137,14 @@ class ExpressionsParser extends Parser {
 
         $.RULE("atomicExpression", (outsideAggregation) => {
             return $.OR([
-                // aggregations not allowed inside other aggregations
-                {GATE: () => outsideAggregation, ALT: () => $.SUBRULE($.aggregationOrMetricExpression, [false]) },
-                // fields not allowed outside aggregations
+                // aggregations and metrics are not allowed inside other aggregations
+                {GATE: () => outsideAggregation, ALT: () => $.SUBRULE($.aggregationExpression, [false]) },
+                {GATE: () => outsideAggregation, ALT: () => $.SUBRULE($.metricExpression) },
+                // fields are not allowed outside aggregations
                 {GATE: () => !outsideAggregation, ALT: () => $.SUBRULE($.fieldExpression) },
                 {ALT: () => $.SUBRULE($.parenthesisExpression, [outsideAggregation]) },
                 {ALT: () => $.SUBRULE($.numberLiteral) }
-            ], "a number or field name");
+            ]);
         });
 
         $.RULE("parenthesisExpression", (outsideAggregation) => {
@@ -168,11 +185,11 @@ class ExpressionsParserMBQL extends ExpressionsParser {
         }
         return initial;
     }
-    _aggregation(aggregation, lParen, args, rParen) {
-        const aggregationName = aggregation.image;
-        return [aggregationsMap.get(aggregationName)].concat(args.values);
+    _aggregation(aggregation, lParen, arg, rParen) {
+        const agg = aggregationsMap.get(aggregation.image)
+        return arg == null ? [agg] : [agg, arg];
     }
-    _metricReference(metricName, metricId) {
+    _metricReference(metricName, lParen, rParen, metricId) {
         return ["METRIC", metricId];
     }
     _fieldReference(fieldName, fieldId) {
@@ -184,7 +201,7 @@ class ExpressionsParserMBQL extends ExpressionsParser {
     _unknownField(fieldName) {
         throw new Error("Unknown field \"" + fieldName + "\"");
     }
-    _unknownMetric(metricName) {
+    _unknownMetric(metricName, lParen, rParen) {
         throw new Error("Unknown metric \"" + metricName + "\"");
     }
 
@@ -207,31 +224,24 @@ class ExpressionsParserMBQL extends ExpressionsParser {
 
 const syntax = (type, ...children) => ({
     type: type,
-    children: children
+    children: children.filter(child => child)
 })
-const token = (token) => ({
+const token = (token) => token && ({
     type: "token",
     text: token.image,
     start: token.startOffset,
     end: token.endOffset,
-})
+});
 
 class ExpressionsParserSyntax extends ExpressionsParser {
     _math(initial, operations) {
         return syntax("math", ...[initial].concat(...operations.map(([op, arg]) => [token(op), arg])));
     }
-    _aggregation(aggregation, lParen, args, rParen) {
-        let argsAndCommas = [];
-        for (let i = 0; i < args.values.length; i++) {
-            argsAndCommas.push(args.values[i]);
-            if (i < args.separators.length) {
-                argsAndCommas.push(args.separators[i]);
-            }
-        }
-        return syntax("aggregation", token(aggregation), token(lParen), ...argsAndCommas, token(rParen));
+    _aggregation(aggregation, lParen, arg, rParen) {
+        return syntax("aggregation", token(aggregation), token(lParen), arg, token(rParen));
     }
-    _metricReference(metricName, metricId) {
-        return syntax("metric", metricName);
+    _metricReference(metricName, lParen, rParen, metricId) {
+        return syntax("metric", metricName, token(lParen), token(rParen));
     }
     _fieldReference(fieldName, fieldId) {
         return syntax("field", fieldName);
@@ -242,8 +252,8 @@ class ExpressionsParserSyntax extends ExpressionsParser {
     _unknownField(fieldName) {
         return syntax("unknown", fieldName);
     }
-    _unknownMetric(metricName) {
-        return syntax("unknown", metricName);
+    _unknownMetric(metricName, lParen, rParen) {
+        return syntax("unknown", metricName, lParen, rParen);
     }
 
     _identifier(identifier) {
@@ -389,17 +399,17 @@ export function suggest(source, {
                     postfixTrim: /^\w+\s*/
                 })));
             }
-        } else if (nextTokenType === Aggregation) {
+        } else if (nextTokenType === Aggregation || nextTokenType === NullaryAggregation || nextTokenType === UnaryAggregation) {
             if (outsideAggregation) {
                 finalSuggestions.push(...tableMetadata.aggregation_options.filter(a => formatAggregationName(a)).map(aggregationOption => {
                     const arity = aggregationOption.fields.length;
                     return {
                         type: "aggregations",
                         name: formatAggregationName(aggregationOption),
-                        text: formatAggregationName(aggregationOption) + "(" + (arity > 0 ? "" : ")"),
-                        postfixText: arity > 0 ? ")" : "",
+                        text: formatAggregationName(aggregationOption) + (arity > 0 ? "(" : " "),
+                        postfixText: (arity > 0 ? ")" : " "),
                         prefixTrim: /\w+$/,
-                        postfixTrim: /^\w+(\(\)?|$)/
+                        postfixTrim: (arity > 0 ? /^\w+(\(\)?|$)/ : /^\w+\s*/)
                     };
                 }));
                 finalSuggestions.push(...tableMetadata.metrics.map(metric => ({

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

@@ -21,6 +21,9 @@ export const VALID_AGGREGATIONS = new Map(Object.entries({
     "max": "Max"
 }));
 
+export const NULLARY_AGGREGATIONS = ["count", "cum_count"];
+export const UNARY_AGGREGATIONS = ["sum", "cum_sum", "distinct", "stddev", "avg", "min", "max"];
+
 export const AdditiveOperator = extendToken("AdditiveOperator", Lexer.NA);
 export const Plus = extendToken("Plus", /\+/, AdditiveOperator);
 export const Minus = extendToken("Minus", /-/, AdditiveOperator);
@@ -30,8 +33,15 @@ export const Multi = extendToken("Multi", /\*/, MultiplicativeOperator);
 export const Div = extendToken("Div", /\//, MultiplicativeOperator);
 
 export const Aggregation = extendToken("Aggregation", Lexer.NA);
-export const aggregationsTokens = Array.from(VALID_AGGREGATIONS).map(([short, expressionName]) =>
-    extendToken(expressionName, new RegExp(expressionName), Aggregation)
+
+export const NullaryAggregation = extendToken("NullaryAggregation", Aggregation);
+const nullaryAggregationTokens = NULLARY_AGGREGATIONS.map((short) =>
+    extendToken(VALID_AGGREGATIONS.get(short), new RegExp(VALID_AGGREGATIONS.get(short), "i"), NullaryAggregation)
+);
+
+export const UnaryAggregation = extendToken("UnaryAggregation", Aggregation);
+const unaryAggregationTokens = UNARY_AGGREGATIONS.map((short) =>
+    extendToken(VALID_AGGREGATIONS.get(short), new RegExp(VALID_AGGREGATIONS.get(short), "i"), UnaryAggregation)
 );
 
 export const Identifier = extendToken('Identifier', /\w+/);
@@ -50,7 +60,9 @@ export const allTokens = [
     WhiteSpace, LParen, RParen, Comma,
     Plus, Minus, Multi, Div,
     AdditiveOperator, MultiplicativeOperator,
-    Aggregation, ...aggregationsTokens,
+    Aggregation,
+    NullaryAggregation, ...nullaryAggregationTokens,
+    UnaryAggregation, ...unaryAggregationTokens,
     StringLiteral, NumberLiteral,
     Identifier
 ];

frontend/test/unit/lib/expressions/formatter.spec.js

@@ -30,13 +30,13 @@ describe("lib/expressions/parser", () => {
         });
 
         it("format aggregations", () => {
-            expect(format(["count"], mockMetadata)).toEqual("Count()");
+            expect(format(["count"], mockMetadata)).toEqual("Count");
             expect(format(["sum", ["field-id", 1]], mockMetadata)).toEqual("Sum(A)");
         });
 
         it("nested aggregation", () => {
-            expect(format(["+", 1, ["count"]], mockMetadata)).toEqual("1 + Count()");
-            expect(format(["/", ["sum", ["field-id", 1]], ["count"]], mockMetadata)).toEqual("Sum(A) / Count()");
+            expect(format(["+", 1, ["count"]], mockMetadata)).toEqual("1 + Count");
+            expect(format(["/", ["sum", ["field-id", 1]], ["count"]], mockMetadata)).toEqual("Sum(A) / Count");
         });
 
         it("aggregation with expressions", () => {

frontend/test/unit/lib/expressions/parser.spec.js

@@ -67,6 +67,7 @@ describe("lib/expressions/parser", () => {
         });
 
         it("can parse aggregation with no arguments", () => {
+            expect(compile("Count", aggregationOpts)).toEqual(["count"]);
             expect(compile("Count()", aggregationOpts)).toEqual(["count"]);
         });
 
@@ -75,7 +76,7 @@ describe("lib/expressions/parser", () => {
         });
 
         it("can parse complex aggregation", () => {
-            expect(compile("1 - Sum(A * 2) / Count()", aggregationOpts)).toEqual(["-", 1, ["/", ["sum", ["*", ["field-id", 1], 2]], ["count"]]]);
+            expect(compile("1 - Sum(A * 2) / Count", aggregationOpts)).toEqual(["-", 1, ["/", ["sum", ["*", ["field-id", 1], 2]], ["count"]]]);
         });
 
         it("should throw exception on invalid input", () => {
@@ -89,7 +90,7 @@ describe("lib/expressions/parser", () => {
     describe("suggest()", () => {
         it("should suggest aggregations and metrics after an operator", () => {
             expect(cleanSuggestions(suggest("1 + ", aggregationOpts))).toEqual([
-                { type: 'aggregations', text: 'Count()' },
+                { type: 'aggregations', text: 'Count ' },
                 { type: 'aggregations', text: 'Sum(' },
                 { type: 'metrics',     text: 'FooBar()' },
                 { type: 'other',       text: ' (' },
@@ -106,7 +107,7 @@ describe("lib/expressions/parser", () => {
         })
         it("should suggest partial matches in aggregation", () => {
             expect(cleanSuggestions(suggest("1 + C", aggregationOpts))).toEqual([
-                { type: 'aggregations', text: 'Count()' },
+                { type: 'aggregations', text: 'Count ' },
             ]);
         })
         it("should suggest partial matches in expression", () => {
@@ -118,19 +119,13 @@ describe("lib/expressions/parser", () => {
 
     describe("compile() in syntax mode", () => {
         it ("should parse source without whitespace into a recoverable syntax tree", () => {
-            const source = "1-Sum(A*2+\"Toucan Sam\")/Count()";
-            const tree = parse(source, aggregationOpts);
-            expect(serialize(tree)).toEqual(source)
-        })
-        xit ("should parse source with metric into a recoverable syntax tree", () => {
-            // FIXME: not preserving parens
-            const source = "Count()+FooBar()";
+            const source = "1-Sum(A*2+\"Toucan Sam\")/Count()+FooBar()";
             const tree = parse(source, aggregationOpts);
             expect(serialize(tree)).toEqual(source)
         })
         xit ("should parse source with whitespace into a recoverable syntax tree", () => {
             // FIXME: not preserving whitespace
-            const source = "1 - Sum(A * 2 + \"Toucan Sam\") / Count()";
+            const source = "1 - Sum(A * 2 + \"Toucan Sam\") / Count + FooBar()";
             const tree = parse(source, aggregationOpts);
             expect(serialize(tree)).toEqual(source)
         })