From d4ccbd4dab19d2d28a2cdfc39aea888f5e0cf2c3 Mon Sep 17 00:00:00 2001
From: Ariya Hidayat <ariya@metabase.com>
Date: Thu, 28 Oct 2021 14:22:30 -0700
Subject: [PATCH] Custom expression editor: less intrusive suggestions (#15688)
The possible matching/choices for function names, dimensions, segments,
and metrics are only shown when there is at least ONE matching character.
Thus, when the user hasn't inputted anything yet, there won't be any
suggestions.
---
.../src/metabase/lib/expressions/suggest.js | 517 +++---------------
.../lib/expressions/suggest.unit.spec.js | 385 ++++---------
.../scenarios/question/filter.cy.spec.js | 6 +-
.../scenarios/question/new.cy.spec.js | 2 +-
.../scenarios/question/notebook.cy.spec.js | 5 +-
5 files changed, 205 insertions(+), 710 deletions(-)
diff --git a/frontend/src/metabase/lib/expressions/suggest.js b/frontend/src/metabase/lib/expressions/suggest.js
index e859f552b68..e4df647a6a8 100644
--- a/frontend/src/metabase/lib/expressions/suggest.js
+++ b/frontend/src/metabase/lib/expressions/suggest.js
@@ -1,79 +1,37 @@
import _ from "underscore";
import {
- getExpressionName,
- // dimensions:
getDimensionName,
formatDimensionName,
- // metrics
formatMetricName,
- // segments
formatSegmentName,
} from "../expressions";
-import {
- parserWithRecovery,
- ExpressionCstVisitor,
- ExpressionParser,
-} from "./parser";
-
-import {
- AggregationFunctionName,
- CLAUSE_TOKENS,
- Case,
- FunctionName,
- Identifier,
- IdentifierString,
- LParen,
- Minus,
- NumberLiteral,
- StringLiteral,
- isTokenType,
- lexerWithRecovery,
-} from "./lexer";
-
import { partialMatch, enclosingFunction } from "./completer";
import getHelpText from "./helper_text_strings";
-import { ExpressionDimension } from "metabase-lib/lib/Dimension";
import {
- FUNCTIONS,
- OPERATORS,
+ EXPRESSION_FUNCTIONS,
+ AGGREGATION_FUNCTIONS,
MBQL_CLAUSES,
- isExpressionType,
- getFunctionArgType,
getMBQLName,
- EXPRESSION_TYPES,
EDITOR_FK_SYMBOLS,
} from "./config";
-const FUNCTIONS_BY_TYPE = {};
-const OPERATORS_BY_TYPE = {};
-for (const type of EXPRESSION_TYPES) {
- FUNCTIONS_BY_TYPE[type] = Array.from(FUNCTIONS)
- .filter(name => isExpressionType(MBQL_CLAUSES[name].type, type))
- .map(name => MBQL_CLAUSES[name]);
- OPERATORS_BY_TYPE[type] = Array.from(OPERATORS)
- .filter(name => isExpressionType(MBQL_CLAUSES[name].type, type))
- .map(name => MBQL_CLAUSES[name]);
-}
-
export function suggest({
source,
- cst,
query,
startRule,
targetOffset = source.length,
- expressionName,
} = {}) {
- const partialSource = source.slice(0, targetOffset);
+ let suggestions = [];
+ const partialSource = source.slice(0, targetOffset);
const matchPrefix = partialMatch(partialSource);
- const partialSuggestionMode =
- matchPrefix && matchPrefix.length > 0 && _.last(matchPrefix) !== "]";
- if (!partialSuggestionMode) {
+ if (!matchPrefix || _.last(matchPrefix) === "]") {
+ // no keystroke to match? show help text for the enclosing function
const functionDisplayName = enclosingFunction(partialSource);
if (functionDisplayName) {
const helpText = getHelpText(getMBQLName(functionDisplayName));
@@ -81,409 +39,106 @@ export function suggest({
return { helpText };
}
}
+ return { suggestions };
}
- const lexResult = lexerWithRecovery.tokenize(partialSource);
- if (lexResult.errors.length > 0) {
- throw lexResult.errors;
- }
- let tokenVector = lexResult.tokens;
- if (partialSuggestionMode) {
- tokenVector = tokenVector.slice(0, -1);
+ if (_.first(matchPrefix) !== "[") {
+ suggestions.push({
+ type: "functions",
+ name: "case",
+ text: "case(",
+ index: targetOffset,
+ });
+ suggestions.push(
+ ...Array.from(EXPRESSION_FUNCTIONS)
+ .map(name => MBQL_CLAUSES[name])
+ .map(func => ({
+ type: "functions",
+ name: func.displayName,
+ text: func.displayName + "(",
+ index: targetOffset,
+ })),
+ );
+ if (startRule === "aggregation") {
+ suggestions.push(
+ ...Array.from(AGGREGATION_FUNCTIONS)
+ .map(name => MBQL_CLAUSES[name])
+ .map(func => ({
+ type: "aggregations",
+ name: func.displayName,
+ text: func.displayName + "(",
+ index: targetOffset,
+ })),
+ );
+ }
}
- const context = getContext({
- cst,
- tokenVector,
- targetOffset,
- startRule,
- }) || { expectedType: startRule };
-
- const { expectedType } = context;
-
- let finalSuggestions = [];
-
- const syntacticSuggestions = parserWithRecovery.computeContentAssist(
- startRule,
- tokenVector,
- );
- for (const suggestion of syntacticSuggestions) {
- const { nextTokenType, ruleStack } = suggestion;
-
- // first to avoid skipping if lastInputTokenIsUnclosedIdentifierString
- if (nextTokenType === Identifier || nextTokenType === IdentifierString) {
- // fields, metrics, segments
- const parentRule = ruleStack.slice(-2, -1)[0];
- const isDimension =
- parentRule === "identifierExpression" &&
- (isExpressionType(expectedType, "expression") ||
- isExpressionType(expectedType, "boolean"));
- const isSegment =
- parentRule === "identifierExpression" &&
- isExpressionType(expectedType, "boolean");
- const isMetric =
- parentRule === "identifierExpression" &&
- isExpressionType(expectedType, "aggregation");
-
- if (isDimension) {
- let dimensions = [];
- if (
- context.token &&
- context.clause &&
- isTokenType(context.token.tokenType, AggregationFunctionName)
- ) {
- dimensions = query.aggregationFieldOptions(context.clause.name).all();
- } else {
- const dimensionFilter = dimension => {
- // not itself
- if (
- dimension instanceof ExpressionDimension &&
- dimension.name() === expressionName
- ) {
- return false;
- }
- if (expectedType === "expression" || expectedType === "boolean") {
- return true;
- }
- const field = dimension.field();
- return (
- (isExpressionType("number", expectedType) && field.isNumeric()) ||
- (isExpressionType("string", expectedType) && field.isString())
- );
- };
- dimensions = query.dimensionOptions(dimensionFilter).all();
- }
- finalSuggestions.push(
- ...dimensions.map(dimension => ({
- type: "fields",
- name: getDimensionName(dimension),
- text: formatDimensionName(dimension) + " ",
- alternates: EDITOR_FK_SYMBOLS.symbols.map(symbol =>
- getDimensionName(dimension, symbol),
- ),
- })),
- );
- }
- if (isSegment) {
- finalSuggestions.push(
- ...query.table().segments.map(segment => ({
- type: "segments",
- name: segment.name,
- text: formatSegmentName(segment),
- })),
- );
- }
- if (isMetric) {
- finalSuggestions.push(
- ...query.table().metrics.map(metric => ({
- type: "metrics",
- name: metric.name,
- text: formatMetricName(metric),
- })),
- );
- }
- } else if (
- isTokenType(nextTokenType, FunctionName) ||
- nextTokenType === Case
- ) {
- const database = query.database();
- let functions = [];
- if (isExpressionType(expectedType, "aggregation")) {
- // special case for aggregation
- finalSuggestions.push(
- // ...query
- // .aggregationOperatorsWithoutRows()
- // .filter(a => getExpressionName(a.short))
- // .map(aggregationOperator =>
- // functionSuggestion(
- // "aggregations",
- // aggregationOperator.short,
- // aggregationOperator.fields.length > 0,
- // ),
- // ),
- ...FUNCTIONS_BY_TYPE["aggregation"]
- .filter(clause => database.hasFeature(clause.requiresFeature))
- .map(clause =>
- functionSuggestion(
- "aggregations",
- clause.name,
- clause.args.length > 0,
- ),
- ),
- );
- finalSuggestions.push(
- ...["sum-where", "count-where", "share"].map(short =>
- functionSuggestion("aggregations", short, true),
+ if (_.last(matchPrefix) !== "]") {
+ suggestions.push(
+ ...query
+ .dimensionOptions(() => true)
+ .all()
+ .map(dimension => ({
+ type: "fields",
+ name: getDimensionName(dimension),
+ text: formatDimensionName(dimension) + " ",
+ alternates: EDITOR_FK_SYMBOLS.symbols.map(symbol =>
+ getDimensionName(dimension, symbol),
),
- );
- functions = FUNCTIONS_BY_TYPE["number"];
- } else {
- functions = FUNCTIONS_BY_TYPE[expectedType];
- }
- finalSuggestions.push(
- ...functions
- .filter(clause => database.hasFeature(clause.requiresFeature))
- .map(clause => functionSuggestion("functions", clause.name)),
+ index: targetOffset,
+ })),
+ );
+ suggestions.push(
+ ...query.table().segments.map(segment => ({
+ type: "segments",
+ name: segment.name,
+ text: formatSegmentName(segment),
+ index: targetOffset,
+ })),
+ );
+ if (startRule === "aggregation") {
+ suggestions.push(
+ ...query.table().metrics.map(metric => ({
+ type: "metrics",
+ name: metric.name,
+ text: formatMetricName(metric),
+ index: targetOffset,
+ })),
);
- if (nextTokenType === Case) {
- const caseSuggestion = {
- type: "functions",
- name: "case",
- text: "case(",
- };
- finalSuggestions.push(caseSuggestion);
- }
- } else if (
- nextTokenType === StringLiteral ||
- nextTokenType === NumberLiteral ||
- nextTokenType === Minus
- ) {
- // skip number/string literal
- } else {
- console.warn("non exhaustive match", nextTokenType.name);
}
}
// throw away any suggestion that is not a suffix of the last partialToken.
- if (partialSuggestionMode) {
- const partial = matchPrefix.toLowerCase();
- for (const suggestion of finalSuggestions) {
- suggestion: for (const text of [
- suggestion.name,
- suggestion.text,
- ...(suggestion.alternates || []),
- ]) {
- const lower = (text || "").toLowerCase();
- if (lower.startsWith(partial)) {
- suggestion.range = [0, partial.length];
+ const partial = matchPrefix.toLowerCase();
+ for (const suggestion of suggestions) {
+ suggestion: for (const text of [
+ suggestion.name,
+ suggestion.text,
+ ...(suggestion.alternates || []),
+ ]) {
+ const lower = (text || "").toLowerCase();
+ if (lower.startsWith(partial)) {
+ suggestion.range = [0, partial.length];
+ break suggestion;
+ }
+ let index = 0;
+ for (const part of lower.split(/\b/g)) {
+ if (part.startsWith(partial)) {
+ suggestion.range = [index, index + partial.length];
break suggestion;
}
- let index = 0;
- for (const part of lower.split(/\b/g)) {
- if (part.startsWith(partial)) {
- suggestion.range = [index, index + partial.length];
- break suggestion;
- }
- index += part.length;
- }
+ index += part.length;
}
}
- finalSuggestions = finalSuggestions.filter(suggestion => suggestion.range);
- }
- for (const suggestion of finalSuggestions) {
- suggestion.index = targetOffset;
- if (!suggestion.name) {
- suggestion.name = suggestion.text;
- }
}
+ suggestions = suggestions.filter(suggestion => suggestion.range);
// deduplicate suggestions and sort by type then name
return {
- suggestions: _.chain(finalSuggestions)
+ suggestions: _.chain(suggestions)
.uniq(suggestion => suggestion.text)
- .sortBy("name")
+ .sortBy("text")
.sortBy("type")
.value(),
};
}
-
-function functionSuggestion(type, clause, parens = true) {
- const name = getExpressionName(clause);
- return {
- type: type,
- name: name,
- text: name + (parens ? "(" : " "),
- };
-}
-
-const contextParser = new ExpressionParser({
- recoveryEnabled: true,
- tokenRecoveryEnabled: false,
-});
-
-export function getContext({
- source,
- cst,
- tokenVector = lexerWithRecovery.tokenize(source).tokens,
- targetOffset = source.length,
- startRule,
- ...options
-}) {
- if (!cst) {
- contextParser.input = tokenVector;
- cst = contextParser[startRule]();
- }
- const visitor = new ExpressionContextVisitor({
- targetOffset: targetOffset,
- tokenVector: tokenVector,
- ...options,
- });
- return visitor.visit(cst);
-}
-
-function findNextTextualToken(tokenVector, prevTokenEndOffset) {
- // The TokenVector is sorted, so we could use a BinarySearch to optimize performance
- const prevTokenIdx = tokenVector.findIndex(
- tok => tok.endOffset === prevTokenEndOffset,
- );
- for (let i = prevTokenIdx + 1; i >= 0 && i < tokenVector.length; i++) {
- if (!/^\s+$/.test(tokenVector[i].image)) {
- return tokenVector[i];
- }
- }
- return null;
-}
-
-export class ExpressionContextVisitor extends ExpressionCstVisitor {
- constructor({ targetOffset, tokenVector }) {
- super();
- this.targetOffset = targetOffset;
- this.tokenVector = tokenVector;
- this.stack = [];
- this.validateVisitor();
- }
-
- _context(clauseToken, index, currentContext) {
- const clause = CLAUSE_TOKENS.get(clauseToken.tokenType);
- let expectedType = getFunctionArgType(clause, index);
-
- if (
- (expectedType === "expression" || expectedType === "number") &&
- currentContext &&
- currentContext.expectedType === "aggregation" &&
- clause.type !== "aggregation"
- ) {
- expectedType = "aggregation";
- }
-
- return { clause, index, expectedType, clauseToken };
- }
-
- _isTarget(token) {
- const { targetOffset, tokenVector } = this;
- const next = findNextTextualToken(tokenVector, token.endOffset);
- if (
- targetOffset > token.endOffset &&
- (next === null || targetOffset <= next.startOffset)
- ) {
- return true;
- }
- return false;
- }
-
- _function(ctx, currentContext) {
- const { tokenVector } = this;
- const clauseToken = ctx.functionName[0];
- // special case: function clauses without parens sometimes causes the paren to be missing
- const parenToken = ctx.LParen
- ? ctx.LParen[0]
- : findNextTextualToken(tokenVector, clauseToken.endOffset);
- if (!parenToken || parenToken.tokenType !== LParen) {
- return;
- }
- const tokens = [parenToken, ...(ctx.Comma || [])];
- for (let index = 0; index < tokens.length; index++) {
- const token = tokens[index];
- if (this._isTarget(token)) {
- return this._context(clauseToken, index, currentContext);
- } else if (ctx.arguments && index < ctx.arguments.length) {
- const match = this.visit(
- ctx.arguments[index],
- this._context(clauseToken, index, currentContext),
- );
- if (match) {
- return match;
- }
- }
- }
- }
-
- _operator(ctx, currentContext) {
- // note: this should probably account for operator precedence / associativity but for now all of our operator clauses contain operators of the same precedence/associativity
- for (let index = 0; index < ctx.operators.length; index++) {
- const clauseToken = ctx.operators[index];
- if (this._isTarget(clauseToken)) {
- // NOTE: operators always (?) have the same type for every operand
- return this._context(clauseToken, 0, currentContext);
- } else {
- const match = this.visit(
- ctx.operands[index],
- this._context(clauseToken, 0, currentContext),
- );
- if (match) {
- return match;
- }
- }
- }
- }
-}
-
-const ALL_RULES = [
- "any",
- "expression",
- "aggregation",
- "boolean",
- "string",
- "number",
- "additionExpression",
- "multiplicationExpression",
- "functionExpression",
- "caseExpression",
- "identifierExpression",
- "identifier",
- "identifierString",
- "stringLiteral",
- "numberLiteral",
- "atomicExpression",
- "parenthesisExpression",
- "booleanExpression",
- "booleanUnaryExpression",
- "relationalExpression",
- "logicalAndExpression",
- "logicalOrExpression",
- "logicalNotExpression",
-];
-
-const TYPE_RULES = new Set([
- "expression",
- "aggregation",
- "boolean",
- "string",
- "number",
-]);
-
-for (const rule of ALL_RULES) {
- ExpressionContextVisitor.prototype[rule] = function(ctx, currentContext) {
- if (!currentContext && TYPE_RULES.has(rule)) {
- currentContext = { expectedType: rule };
- }
-
- // if we have operators or a functionName then handle that specially
- if (ctx.operators) {
- const match = this._operator(ctx, currentContext);
- if (match) {
- return match;
- }
- }
- if (ctx.functionName) {
- const match = this._function(ctx, currentContext);
- if (match) {
- return match;
- }
- }
-
- // this just visits every child
- for (const type in ctx) {
- for (const child of ctx[type]) {
- if (!child.tokenType && child.name) {
- const match = this.visit(child, currentContext);
- if (match) {
- return match;
- }
- } else if (this._isTarget(child)) {
- return currentContext;
- }
- }
- }
- };
-}
diff --git a/frontend/test/metabase/lib/expressions/suggest.unit.spec.js b/frontend/test/metabase/lib/expressions/suggest.unit.spec.js
index 7467e5fd11c..dd2ec0097d0 100644
--- a/frontend/test/metabase/lib/expressions/suggest.unit.spec.js
+++ b/frontend/test/metabase/lib/expressions/suggest.unit.spec.js
@@ -1,95 +1,11 @@
-import {
- suggest as suggest_,
- getContext as getContext_,
-} from "metabase/lib/expressions/suggest";
+import { suggest as suggest_ } from "metabase/lib/expressions/suggest";
import _ from "underscore";
-import {
- aggregationOpts,
- expressionOpts,
- filterOpts,
-} from "./__support__/expressions";
+import { aggregationOpts, expressionOpts } from "./__support__/expressions";
import { ORDERS, REVIEWS } from "__support__/sample_dataset_fixture";
-const AGGREGATION_FUNCTIONS = [
- { type: "aggregations", text: "Average(" },
- { type: "aggregations", text: "Count " },
- { type: "aggregations", text: "CountIf(" },
- { type: "aggregations", text: "CumulativeCount " },
- { type: "aggregations", text: "CumulativeSum(" },
- { type: "aggregations", text: "Distinct(" },
- { type: "aggregations", text: "Max(" },
- { type: "aggregations", text: "Median(" },
- { type: "aggregations", text: "Min(" },
- { type: "aggregations", text: "Percentile(" },
- { type: "aggregations", text: "Share(" },
- { type: "aggregations", text: "StandardDeviation(" },
- { type: "aggregations", text: "Sum(" },
- { type: "aggregations", text: "SumIf(" },
- { type: "aggregations", text: "Variance(" },
-];
-const STRING_FUNCTIONS = [
- { text: "concat(", type: "functions" },
- { text: "lower(", type: "functions" },
- { text: "ltrim(", type: "functions" },
- { text: "regexextract(", type: "functions" },
- { text: "rtrim(", type: "functions" },
- { text: "replace(", type: "functions" },
- { text: "substring(", type: "functions" },
- { text: "trim(", type: "functions" },
- { text: "upper(", type: "functions" },
-];
-const NUMERIC_FUNCTIONS = [
- { text: "abs(", type: "functions" },
- { text: "ceil(", type: "functions" },
- { text: "exp(", type: "functions" },
- { text: "floor(", type: "functions" },
- { text: "length(", type: "functions" },
- { text: "log(", type: "functions" },
- { text: "power(", type: "functions" },
- { text: "round(", type: "functions" },
- { text: "sqrt(", type: "functions" },
-];
-
-const STRING_FUNCTIONS_EXCLUDING_REGEX = STRING_FUNCTIONS.filter(
- ({ text }) => text !== "regexextract(",
-);
-// const EXPRESSION_FUNCTIONS = [
-// { text: "case(", type: "functions" },
-// { text: "coalesce(", type: "functions" },
-// ];
-const FILTER_FUNCTIONS = [
- { text: "between(", type: "functions" },
- { text: "contains(", type: "functions" },
- { text: "endsWith(", type: "functions" },
- { text: "interval(", type: "functions" },
- { text: "isempty(", type: "functions" },
- { text: "isnull(", type: "functions" },
- { text: "startsWith(", type: "functions" },
-];
-
-// custom metadata defined in __support__/expressions
-const METRICS_CUSTOM = [{ type: "metrics", text: "[metric]" }];
-const FIELDS_CUSTOM = [
- { type: "fields", text: "[A] " },
- { type: "fields", text: "[B] " },
- { type: "fields", text: "[C] " },
- // quoted because conflicts with aggregation
- { type: "fields", text: "[Sum] " },
- // quoted because has a space
- { type: "fields", text: "[Toucan Sam] " },
- // quoted because conflicts with aggregation
- { type: "fields", text: "[count] " },
-];
-
-const FIELDS_CUSTOM_NON_NUMERIC = [
- { type: "fields", text: "[date] " },
- { type: "fields", text: "[text] " },
-];
-
// custom metadata defined in __support__/sample_dataset_fixture
-const METRICS_ORDERS = [{ type: "metrics", text: "[Total Order Value]" }];
const SEGMENTS_ORDERS = [{ text: "[Expensive Things]", type: "segments" }];
const FIELDS_ORDERS = [
{ text: "[Created At] ", type: "fields" },
@@ -122,6 +38,25 @@ const FIELDS_ORDERS = [
{ text: "[User → Zip] ", type: "fields" },
];
+// custom metadata defined in __support__/expressions
+const METRICS_CUSTOM = [{ type: "metrics", text: "[metric]" }];
+const FIELDS_CUSTOM = [
+ { type: "fields", text: "[A] " },
+ { type: "fields", text: "[B] " },
+ { type: "fields", text: "[C] " },
+ // quoted because conflicts with aggregation
+ { type: "fields", text: "[Sum] " },
+ // quoted because has a space
+ { type: "fields", text: "[Toucan Sam] " },
+ // quoted because conflicts with aggregation
+ { type: "fields", text: "[count] " },
+];
+
+const FIELDS_CUSTOM_NON_NUMERIC = [
+ { type: "fields", text: "[date] " },
+ { type: "fields", text: "[text] " },
+];
+
describe("metabase/lib/expression/suggest", () => {
describe("suggest()", () => {
function suggest(...args) {
@@ -133,41 +68,26 @@ describe("metabase/lib/expression/suggest", () => {
}
describe("expression", () => {
- it("should suggest expression functions, and fields in an expression", () => {
- expect(suggest({ source: "", ...expressionOpts })).toEqual([
- ...FIELDS_CUSTOM,
- ...FIELDS_CUSTOM_NON_NUMERIC,
- ...[
- { type: "functions", text: "case(" },
- { type: "functions", text: "coalesce(" },
- ...NUMERIC_FUNCTIONS,
- ...STRING_FUNCTIONS_EXCLUDING_REGEX,
- ].sort(suggestionSort),
- ]);
- });
-
- it("should suggest numeric fields after an aritmetic", () => {
- expect(suggest({ source: "1 + ", ...expressionOpts })).toEqual([
- ...FIELDS_CUSTOM,
- ...[{ type: "functions", text: "case(" }, ...NUMERIC_FUNCTIONS].sort(
- suggestionSort,
- ),
- ]);
- });
it("should suggest partial matches in expression", () => {
expect(suggest({ source: "1 + C", ...expressionOpts })).toEqual([
{ type: "fields", text: "[C] " },
{ type: "fields", text: "[count] " },
{ type: "functions", text: "case(" },
{ type: "functions", text: "ceil(" },
+ // FIXME: the last three should not appear
+ { type: "functions", text: "coalesce(" },
+ { type: "functions", text: "concat(" },
+ { type: "functions", text: "contains(" },
]);
});
+
it("should suggest partial matches in unterminated quoted string", () => {
expect(suggest({ source: "1 + [C", ...expressionOpts })).toEqual([
{ type: "fields", text: "[C] " },
{ type: "fields", text: "[count] " },
]);
});
+
it("should suggest foreign fields", () => {
expect(
suggest({
@@ -192,6 +112,7 @@ describe("metabase/lib/expression/suggest", () => {
{ text: "[User → Zip] ", type: "fields" },
]);
});
+
it("should suggest joined fields", () => {
expect(
suggest({
@@ -211,10 +132,11 @@ describe("metabase/lib/expression/suggest", () => {
{ text: "[Foo → Reviewer] ", type: "fields" },
]);
});
+
it("should suggest nested query fields", () => {
expect(
suggest({
- source: "",
+ source: "T",
query: ORDERS.query()
.aggregate(["count"])
.breakout(ORDERS.TOTAL)
@@ -223,12 +145,8 @@ describe("metabase/lib/expression/suggest", () => {
}),
).toEqual(
[
- { text: "[Count] ", type: "fields" },
{ text: "[Total] ", type: "fields" },
- { type: "functions", text: "case(" },
- { text: "coalesce(", type: "functions" },
- ...STRING_FUNCTIONS,
- ...NUMERIC_FUNCTIONS,
+ { text: "trim(", type: "functions" },
].sort(suggestionSort),
);
});
@@ -262,67 +180,80 @@ describe("metabase/lib/expression/suggest", () => {
}).name,
).toEqual("coalesce");
});
-
- xit("should suggest boolean options after case(", () => {
- expect(
- suggest({
- source: "case(",
- query: ORDERS.query(),
- startRule: "expression",
- }),
- ).toEqual([...SEGMENTS_ORDERS]);
- });
});
describe("aggregation", () => {
+ it("should suggest aggregations and metrics", () => {
+ expect(suggest({ source: "case([", ...aggregationOpts })).toEqual(
+ [
+ ...FIELDS_CUSTOM,
+ ...METRICS_CUSTOM,
+ ...FIELDS_CUSTOM_NON_NUMERIC,
+ { type: "segments", text: "[segment]" },
+ ].sort(suggestionSort),
+ );
+ });
+
it("should suggest partial matches after an aggregation", () => {
expect(suggest({ source: "average(c", ...aggregationOpts })).toEqual([
+ // FIXME: the next four should not appear
+ { type: "aggregations", text: "Count(" },
+ { type: "aggregations", text: "CountIf(" },
+ { type: "aggregations", text: "CumulativeCount(" },
+ { type: "aggregations", text: "CumulativeSum(" },
{ type: "fields", text: "[C] " },
{ type: "fields", text: "[count] " },
- { text: "case(", type: "functions" },
- // { text: "coalesce(", type: "functions" },
- { text: "ceil(", type: "functions" },
- ]);
- });
- it("should suggest aggregations and metrics after an operator", () => {
- expect(suggest({ source: "1 + ", ...aggregationOpts })).toEqual([
- ...[
- { type: "functions", text: "case(" },
- ...AGGREGATION_FUNCTIONS,
- ...NUMERIC_FUNCTIONS,
- ].sort(suggestionSort),
- ...METRICS_CUSTOM,
+ { type: "functions", text: "case(" },
+ { type: "functions", text: "ceil(" },
+ { type: "functions", text: "coalesce(" },
+ { type: "functions", text: "concat(" },
+ { type: "functions", text: "contains(" },
]);
});
+
it("should suggest partial matches in aggregation", () => {
expect(suggest({ source: "1 + C", ...aggregationOpts })).toEqual([
- { type: "aggregations", text: "Count " },
+ { type: "aggregations", text: "Count(" },
{ type: "aggregations", text: "CountIf(" },
- { type: "aggregations", text: "CumulativeCount " },
+ { type: "aggregations", text: "CumulativeCount(" },
{ type: "aggregations", text: "CumulativeSum(" },
+ { type: "fields", text: "[C] " },
+ { type: "fields", text: "[count] " },
{ type: "functions", text: "case(" },
{ type: "functions", text: "ceil(" },
+ { type: "functions", text: "coalesce(" },
+ { type: "functions", text: "concat(" },
+ { type: "functions", text: "contains(" },
]);
});
- it("should suggest aggregations and metrics in an aggregation", () => {
+ it("should show suggestions with matched 2-char prefix", () => {
expect(
suggest({
- source: "",
+ source: "to",
query: ORDERS.query(),
startRule: "aggregation",
}),
).toEqual([
- ...[
- { type: "functions", text: "case(" },
- ...AGGREGATION_FUNCTIONS,
- ...NUMERIC_FUNCTIONS,
- ].sort(suggestionSort),
- ...METRICS_ORDERS,
+ { type: "metrics", text: "[Total Order Value]" },
+ { type: "fields", text: "[Total] " },
+ ]);
+ });
+
+ it("should show suggestions with matched 3-char prefix", () => {
+ expect(
+ suggest({
+ source: "cou",
+ query: ORDERS.query(),
+ startRule: "aggregation",
+ }),
+ ).toEqual([
+ { type: "aggregations", text: "Count(" },
+ { type: "aggregations", text: "CountIf(" },
]);
});
- it("should show help text in an aggregation functiom", () => {
+ it("should show help text in an aggregation function", () => {
const { name, example } = helpText({
source: "Sum(",
query: ORDERS.query(),
@@ -334,18 +265,42 @@ describe("metabase/lib/expression/suggest", () => {
});
describe("filter", () => {
- it("should suggest filter functions, fields, and segments in a filter", () => {
+ it("should show suggestions with matched 1-char prefix", () => {
+ expect(
+ suggest({ source: "c", query: ORDERS.query(), startRule: "boolean" }),
+ ).toEqual([
+ { type: "fields", text: "[Created At] " },
+ { type: "fields", text: "[Product → Category] " },
+ { type: "fields", text: "[Product → Created At] " },
+ { type: "fields", text: "[User → City] " },
+ { type: "fields", text: "[User → Created At] " },
+ { type: "functions", text: "case(" },
+ { type: "functions", text: "ceil(" },
+ { type: "functions", text: "coalesce(" },
+ { type: "functions", text: "concat(" },
+ { type: "functions", text: "contains(" },
+ ]);
+ });
+
+ it("should show suggestions with matched 2-char prefix", () => {
expect(
- suggest({ source: "", query: ORDERS.query(), startRule: "boolean" }),
+ suggest({
+ source: "ca",
+ query: ORDERS.query(),
+ startRule: "boolean",
+ }),
).toEqual([
- ...FIELDS_ORDERS,
- ...[{ type: "functions", text: "case(" }, ...FILTER_FUNCTIONS].sort(
- suggestionSort,
- ),
- ...SEGMENTS_ORDERS,
+ { type: "fields", text: "[Product → Category] " },
+ { type: "functions", text: "case(" },
]);
});
+ it("should show all fields when '[' appears", () => {
+ expect(
+ suggest({ source: "[", query: ORDERS.query(), startRule: "boolean" }),
+ ).toEqual([...FIELDS_ORDERS, ...SEGMENTS_ORDERS].sort(suggestionSort));
+ });
+
it("should show help text in a filter function", () => {
const { name, example } = helpText({
source: "Contains(Total ",
@@ -357,134 +312,14 @@ describe("metabase/lib/expression/suggest", () => {
});
});
});
-
- describe("getContext", () => {
- function getContext(...args) {
- return cleanContext(getContext_(...args));
- }
-
- describe("aggregation", () => {
- it("should get operator context", () => {
- expect(getContext({ source: "1 +", ...aggregationOpts })).toEqual({
- clause: "+",
- expectedType: "aggregation",
- index: 0,
- });
- });
- it("should get operator context with trailing whitespace", () => {
- expect(getContext({ source: "1 + ", ...aggregationOpts })).toEqual({
- clause: "+",
- expectedType: "aggregation",
- index: 0,
- });
- });
- it("should get aggregation context", () => {
- expect(getContext({ source: "Average(", ...aggregationOpts })).toEqual({
- clause: "avg",
- expectedType: "number",
- index: 0,
- });
- });
- it("should get aggregation context with closing paren", () => {
- expect(
- getContext({
- source: "Average()",
- ...aggregationOpts,
- targetOffset: 8,
- }),
- ).toEqual({
- clause: "avg",
- expectedType: "number",
- index: 0,
- });
- });
- it("should get sum-where first argument", () => {
- expect(
- getContext({ source: "1 + SumIf(", ...aggregationOpts }),
- ).toEqual({
- clause: "sum-where",
- expectedType: "number",
- index: 0,
- });
- });
- it("should get sum-where second argument", () => {
- expect(
- getContext({ source: "1 + SumIf(Total = 10,", ...aggregationOpts }),
- ).toEqual({
- clause: "sum-where",
- expectedType: "boolean",
- index: 1,
- });
- });
- it("should get operator context inside aggregation", () => {
- expect(
- getContext({ source: "1 + Sum(2 /", ...aggregationOpts }),
- ).toEqual({
- clause: "/",
- expectedType: "number",
- index: 0,
- });
- });
- });
- describe("expression", () => {
- it("should get operator context", () => {
- expect(getContext({ source: "1 +", ...expressionOpts })).toEqual({
- clause: "+",
- expectedType: "number",
- index: 0,
- });
- });
- it("should get function context", () => {
- expect(getContext({ source: "trim(", ...expressionOpts })).toEqual({
- clause: "trim",
- expectedType: "string",
- index: 0,
- });
- });
- xit("should get boolean for first argument of case", () => {
- // it's difficult to type "case" correctly using the current system because the form is:
- // case([PREDICATE, EXPRESSION]+ [, ELSE-EXPRESSION]?)
- expect(getContext({ source: "case(", ...expressionOpts })).toEqual({
- clause: "case",
- expectedType: "boolean",
- index: 0,
- });
- });
- it("should get expression for second argument of case", () => {
- expect(getContext({ source: "case(Foo,", ...expressionOpts })).toEqual({
- clause: "case",
- expectedType: "expression",
- index: 1,
- });
- });
- });
- describe("filter", () => {
- it("should get function context", () => {
- expect(getContext({ source: "between(", ...filterOpts })).toEqual({
- clause: "between",
- expectedType: "expression",
- index: 0,
- });
- });
- });
- });
});
-function cleanContext(context) {
- delete context.clauseToken;
- if (context.clause) {
- context.clause = context.clause.name;
- }
- return context;
-}
-
function cleanSuggestions(suggestions) {
return _.chain(suggestions)
.map(s => _.pick(s, "type", "text"))
.sortBy("text")
- .sortBy("type")
.value();
}
const suggestionSort = (a, b) =>
- a.type.localeCompare(b.type) || a.text.localeCompare(b.text);
+ a.text < b.text ? -1 : a.text > b.text ? 1 : 0;
diff --git a/frontend/test/metabase/scenarios/question/filter.cy.spec.js b/frontend/test/metabase/scenarios/question/filter.cy.spec.js
index b6061919bb9..958486c4d38 100644
--- a/frontend/test/metabase/scenarios/question/filter.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/filter.cy.spec.js
@@ -369,6 +369,7 @@ describe("scenarios > question > filter", () => {
openProductsTable();
cy.findByText("Filter").click();
cy.findByText("Custom Expression").click();
+ typeInExpressionEditor("c");
// This issue has two problematic parts. We're testing for both:
cy.log("Popover should display all custom expression options");
@@ -380,7 +381,7 @@ describe("scenarios > question > filter", () => {
cy.log("Should not display error prematurely");
cy.get("[contenteditable='true']")
.click()
- .type("contains(");
+ .type("ontains(");
cy.findByText(/Checks to see if string1 contains string2 within it./i);
cy.button("Done").should("not.be.disabled");
cy.get(".text-error").should("not.exist");
@@ -473,9 +474,10 @@ describe("scenarios > question > filter", () => {
it("should offer case expression in the auto-complete suggestions", () => {
openExpressionEditorFromFreshlyLoadedPage();
+ typeInExpressionEditor("c");
popover().contains(/case/i);
- typeInExpressionEditor("c");
+ typeInExpressionEditor("a");
// "case" is still there after typing a bit
popover().contains(/case/i);
diff --git a/frontend/test/metabase/scenarios/question/new.cy.spec.js b/frontend/test/metabase/scenarios/question/new.cy.spec.js
index 40622ea1a96..c594c00093a 100644
--- a/frontend/test/metabase/scenarios/question/new.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/new.cy.spec.js
@@ -462,7 +462,7 @@ describe("scenarios > question > new", () => {
});
});
- it.skip("distinct inside custom expression should suggest non-numeric types (metabase#13469)", () => {
+ it("distinct inside custom expression should suggest non-numeric types (metabase#13469)", () => {
openReviewsTable({ mode: "notebook" });
cy.findByText("Summarize").click();
popover()
diff --git a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
index 0bcb90cfadb..7dc0240d23c 100644
--- a/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
+++ b/frontend/test/metabase/scenarios/question/notebook.cy.spec.js
@@ -894,8 +894,11 @@ function joinTwoSavedQuestions() {
}
function addSimpleCustomColumn(name) {
+ cy.get("[contenteditable='true']")
+ .click()
+ .type("C");
popover()
- .findByText("Category")
+ .findByText("ategory")
.click();
cy.findByPlaceholderText("Something nice and descriptive")
.click()
--
GitLab