From 4a76c4b05395dd0033491b4b48ea41b42c913bb0 Mon Sep 17 00:00:00 2001
From: Ariya Hidayat <ariya@metabase.com>
Date: Fri, 11 Feb 2022 08:49:15 -0800
Subject: [PATCH] Fix parsing of some aggregation functions with no args
(#20432)
In an expression `COUNT/2`, `COUNT` there should be treated as a
function (i.e. `COUNT()`) instead of a field.
Another function is the same category is `CUMULATIVECOUNT`.
Both don't require any arguments, hence the parentheses are optional.
---
.../metabase/lib/expressions/diagnostics.js | 7 ++++++-
.../lib/expressions/recursive-parser.js | 19 +++++++++++++++++++
.../src/metabase/lib/expressions/resolver.js | 3 +++
.../expressions/recursive-parser.unit.spec.js | 5 +++++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/frontend/src/metabase/lib/expressions/diagnostics.js b/frontend/src/metabase/lib/expressions/diagnostics.js
index edc6801c1ff..7e7627b74fe 100644
--- a/frontend/src/metabase/lib/expressions/diagnostics.js
+++ b/frontend/src/metabase/lib/expressions/diagnostics.js
@@ -1,6 +1,7 @@
import { t } from "ttag";
import {
+ MBQL_CLAUSES,
getMBQLName,
parseDimension,
parseMetric,
@@ -17,6 +18,7 @@ import {
useShorthands,
adjustCase,
adjustOptions,
+ transformNoArgFunction,
} from "metabase/lib/expressions/recursive-parser";
import { tokenize, TOKEN, OPERATOR } from "metabase/lib/expressions/tokenizer";
@@ -43,7 +45,9 @@ export function diagnose(source, startRule, query) {
const token = tokens[i];
if (token.type === TOKEN.Identifier && source[token.start] !== "[") {
const functionName = source.slice(token.start, token.end);
- if (getMBQLName(functionName)) {
+ const fn = getMBQLName(functionName);
+ const clause = fn ? MBQL_CLAUSES[fn] : null;
+ if (clause && clause.args.length > 0) {
const next = tokens[i + 1];
if (next.op !== OPERATOR.OpenParenthesis) {
return {
@@ -121,6 +125,7 @@ function prattCompiler(source, startRule, query) {
passes: [
adjustOptions,
useShorthands,
+ transformNoArgFunction,
adjustCase,
expr => resolve(expr, startRule, resolveMBQLField),
],
diff --git a/frontend/src/metabase/lib/expressions/recursive-parser.js b/frontend/src/metabase/lib/expressions/recursive-parser.js
index 0f6328861c4..d7287b6febe 100644
--- a/frontend/src/metabase/lib/expressions/recursive-parser.js
+++ b/frontend/src/metabase/lib/expressions/recursive-parser.js
@@ -303,11 +303,30 @@ export const adjustCase = tree =>
return node;
});
+// [ "dimension", "count "] becomes ["count"], since Count is a valid no-arg function
+export const transformNoArgFunction = tree =>
+ modify(tree, node => {
+ if (Array.isArray(node)) {
+ const [operator, ...operands] = node;
+ if (operands.length === 1 && operator === "dimension") {
+ const text = operands[0];
+ const fn = getMBQLName(text.trim().toLowerCase());
+
+ // refering a function with no args?
+ if (fn && MBQL_CLAUSES[fn].args.length === 0) {
+ return [fn];
+ }
+ }
+ }
+ return node;
+ });
+
const pipe = (...fns) => x => fns.reduce((v, f) => f(v), x);
export const parse = pipe(
recursiveParse,
adjustOptions,
useShorthands,
+ transformNoArgFunction,
adjustCase,
);
diff --git a/frontend/src/metabase/lib/expressions/resolver.js b/frontend/src/metabase/lib/expressions/resolver.js
index 5fed295a3a7..fa9b3fc75c1 100644
--- a/frontend/src/metabase/lib/expressions/resolver.js
+++ b/frontend/src/metabase/lib/expressions/resolver.js
@@ -48,6 +48,9 @@ const isCompatible = (a, b) => {
if (a === "aggregation" && b === "number") {
return true;
}
+ if (a === "number" && b === "aggregation") {
+ return true;
+ }
return false;
};
diff --git a/frontend/test/metabase/lib/expressions/recursive-parser.unit.spec.js b/frontend/test/metabase/lib/expressions/recursive-parser.unit.spec.js
index 39b44da6851..1504b3c3521 100644
--- a/frontend/test/metabase/lib/expressions/recursive-parser.unit.spec.js
+++ b/frontend/test/metabase/lib/expressions/recursive-parser.unit.spec.js
@@ -149,6 +149,11 @@ describe("metabase/lib/expressions/recursive-parser", () => {
expect(process("A and not X")).toEqual(["and", A, ["not", X]]);
});
+ it("should detect aggregation functions with no argument", () => {
+ expect(process("COUNT/2")).toEqual(["/", ["count"], 2]);
+ expect(process("1+CumulativeCount")).toEqual(["+", 1, ["cum-count"]]);
+ });
+
it("should resolve segments", () => {
expect(process("Expensive", "boolean")).toEqual(["segment", "Expensive"]);
expect(process("NOT LowVolume")).toEqual(["not", ["segment", "LowVolume"]]);
--
GitLab