From 9f1e07d65ec653469126ba506d06c2f01b308805 Mon Sep 17 00:00:00 2001
From: Tom Robinson <tlrobinson@gmail.com>
Date: Wed, 14 Dec 2016 14:11:36 -0800
Subject: [PATCH] Update parser/formatter to use string literal or identifier
for metric names
---
.../src/metabase/lib/expressions/formatter.js | 2 +-
.../src/metabase/lib/expressions/index.js | 3 +-
.../src/metabase/lib/expressions/parser.js | 46 ++++++++++---------
.../unit/lib/expressions/formatter.spec.js | 2 +-
.../test/unit/lib/expressions/parser.spec.js | 6 +--
5 files changed, 31 insertions(+), 28 deletions(-)
diff --git a/frontend/src/metabase/lib/expressions/formatter.js b/frontend/src/metabase/lib/expressions/formatter.js
index 84c0645f6d0..c089d8cd1d1 100644
--- a/frontend/src/metabase/lib/expressions/formatter.js
+++ b/frontend/src/metabase/lib/expressions/formatter.js
@@ -56,7 +56,7 @@ function formatMetric([, metricId], { tableMetadata: { metrics } }) {
if (!metric) {
throw 'metric with ID does not exist: ' + metricId;
}
- return formatMetricName(metric) + "()";
+ return formatMetricName(metric);
}
function formatExpressionReference([, expressionName]) {
diff --git a/frontend/src/metabase/lib/expressions/index.js b/frontend/src/metabase/lib/expressions/index.js
index 39a549a9d27..e12463eba03 100644
--- a/frontend/src/metabase/lib/expressions/index.js
+++ b/frontend/src/metabase/lib/expressions/index.js
@@ -1,7 +1,6 @@
import _ from "underscore";
import { mbqlEq } from "../query/util";
-import { titleize } from "../formatting";
import { VALID_OPERATORS, VALID_AGGREGATIONS } from "./tokens";
@@ -18,7 +17,7 @@ function formatIdentifier(name) {
}
export function formatMetricName(metric) {
- return titleize(metric.name).replace(/\W+/g, "")
+ return formatIdentifier(metric.name);
}
export function formatFieldName(field) {
diff --git a/frontend/src/metabase/lib/expressions/parser.js b/frontend/src/metabase/lib/expressions/parser.js
index 44ce34131e9..bed0d71e813 100644
--- a/frontend/src/metabase/lib/expressions/parser.js
+++ b/frontend/src/metabase/lib/expressions/parser.js
@@ -7,7 +7,7 @@ import { formatFieldName, formatMetricName, formatExpressionName, formatAggregat
import {
VALID_AGGREGATIONS,
allTokens,
- LParen, RParen, Comma,
+ LParen, RParen,
AdditiveOperator, MultiplicativeOperator,
Aggregation, NullaryAggregation, UnaryAggregation,
StringLiteral, NumberLiteral,
@@ -20,7 +20,14 @@ const aggregationsMap = new Map(Array.from(VALID_AGGREGATIONS).map(([a,b]) => [b
class ExpressionsParser extends Parser {
constructor(input, options = {}) {
- super(input, allTokens/*, { recoveryEnabled: false }*/);
+ const parserOptions = {
+ // recoveryEnabled: false,
+ ignoredIssues: {
+ // uses GATE to disambiguate fieldName and metricName
+ atomicExpression: { OR1: true }
+ }
+ };
+ super(input, allTokens, parserOptions);
let $ = this;
@@ -88,19 +95,16 @@ class ExpressionsParser extends Parser {
});
$.RULE("metricExpression", () => {
- const metricName = $.SUBRULE($.identifier);
- const lParen = $.CONSUME(LParen);
- const rParen = $.CONSUME(RParen);
- // const metricName = $.OR([
- // {ALT: () => $.SUBRULE($.stringLiteral) },
- // {ALT: () => $.SUBRULE($.identifier) }
- // ]);
+ const metricName = $.OR([
+ {ALT: () => $.SUBRULE($.stringLiteral) },
+ {ALT: () => $.SUBRULE($.identifier) }
+ ]);
const metric = this.getMetricForName(this._toString(metricName));
if (metric != null) {
- return this._metricReference(metricName, lParen, rParen, metric.id);
+ return this._metricReference(metricName, metric.id);
}
- return this._unknownMetric(metricName, lParen, rParen);
+ return this._unknownMetric(metricName);
});
$.RULE("fieldExpression", () => {
@@ -169,7 +173,7 @@ class ExpressionsParser extends Parser {
getMetricForName(metricName) {
const metrics = this._options.tableMetadata && this._options.tableMetadata.metrics;
- return _.find(metrics, (metric) => formatMetricName(metric) === metricName);
+ return _.find(metrics, (metric) => metric.name.toLowerCase() === metricName.toLowerCase());
}
}
@@ -189,7 +193,7 @@ class ExpressionsParserMBQL extends ExpressionsParser {
const agg = aggregationsMap.get(aggregation.image)
return arg == null ? [agg] : [agg, arg];
}
- _metricReference(metricName, lParen, rParen, metricId) {
+ _metricReference(metricName, metricId) {
return ["METRIC", metricId];
}
_fieldReference(fieldName, fieldId) {
@@ -201,7 +205,7 @@ class ExpressionsParserMBQL extends ExpressionsParser {
_unknownField(fieldName) {
throw new Error("Unknown field \"" + fieldName + "\"");
}
- _unknownMetric(metricName, lParen, rParen) {
+ _unknownMetric(metricName) {
throw new Error("Unknown metric \"" + metricName + "\"");
}
@@ -240,8 +244,8 @@ class ExpressionsParserSyntax extends ExpressionsParser {
_aggregation(aggregation, lParen, arg, rParen) {
return syntax("aggregation", token(aggregation), token(lParen), arg, token(rParen));
}
- _metricReference(metricName, lParen, rParen, metricId) {
- return syntax("metric", metricName, token(lParen), token(rParen));
+ _metricReference(metricName, metricId) {
+ return syntax("metric", metricName);
}
_fieldReference(fieldName, fieldId) {
return syntax("field", fieldName);
@@ -252,8 +256,8 @@ class ExpressionsParserSyntax extends ExpressionsParser {
_unknownField(fieldName) {
return syntax("unknown", fieldName);
}
- _unknownMetric(metricName, lParen, rParen) {
- return syntax("unknown", metricName, lParen, rParen);
+ _unknownMetric(metricName) {
+ return syntax("unknown", metricName);
}
_identifier(identifier) {
@@ -399,7 +403,7 @@ export function suggest(source, {
postfixTrim: /^\w+\s*/
})));
}
- } else if (nextTokenType === Aggregation || nextTokenType === NullaryAggregation || nextTokenType === UnaryAggregation) {
+ } else if (nextTokenType === Aggregation || nextTokenType === NullaryAggregation || nextTokenType === UnaryAggregation || nextTokenType === Identifier || nextTokenType === StringLiteral) {
if (outsideAggregation) {
finalSuggestions.push(...tableMetadata.aggregation_options.filter(a => formatAggregationName(a)).map(aggregationOption => {
const arity = aggregationOption.fields.length;
@@ -415,9 +419,9 @@ export function suggest(source, {
finalSuggestions.push(...tableMetadata.metrics.map(metric => ({
type: "metrics",
name: metric.name,
- text: formatMetricName(metric) + "()",
+ text: formatMetricName(metric),
prefixTrim: /\w+$/,
- postfixTrim: /^\w+(\(\)?|$)/
+ postfixTrim: /^\w+\s*/
})))
}
} else if (nextTokenType === NumberLiteral) {
diff --git a/frontend/test/unit/lib/expressions/formatter.spec.js b/frontend/test/unit/lib/expressions/formatter.spec.js
index e56204e7159..0cd7715fcd1 100644
--- a/frontend/test/unit/lib/expressions/formatter.spec.js
+++ b/frontend/test/unit/lib/expressions/formatter.spec.js
@@ -44,7 +44,7 @@ describe("lib/expressions/parser", () => {
});
it("expression with metric", () => {
- expect(format(["+", 1, ["METRIC", 1]], mockMetadata)).toEqual("1 + FooBar()");
+ expect(format(["+", 1, ["METRIC", 1]], mockMetadata)).toEqual("1 + \"foo bar\"");
});
it("expression with custom field", () => {
diff --git a/frontend/test/unit/lib/expressions/parser.spec.js b/frontend/test/unit/lib/expressions/parser.spec.js
index a3a7d020dab..b35b2763aeb 100644
--- a/frontend/test/unit/lib/expressions/parser.spec.js
+++ b/frontend/test/unit/lib/expressions/parser.spec.js
@@ -92,7 +92,7 @@ describe("lib/expressions/parser", () => {
expect(cleanSuggestions(suggest("1 + ", aggregationOpts))).toEqual([
{ type: 'aggregations', text: 'Count ' },
{ type: 'aggregations', text: 'Sum(' },
- { type: 'metrics', text: 'FooBar()' },
+ { type: 'metrics', text: '"foo bar"' },
{ type: 'other', text: ' (' },
]);
})
@@ -119,13 +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()+FooBar()";
+ const source = "1-Sum(A*2+\"Toucan Sam\")/Count()+\"foo bar\"";
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 + FooBar()";
+ const source = "1 - Sum(A * 2 + \"Toucan Sam\") / Count + \"foo bar\"";
const tree = parse(source, aggregationOpts);
expect(serialize(tree)).toEqual(source)
})
--
GitLab