From 3db6407f637ad986fa9a1cd8f9f1831595e278b4 Mon Sep 17 00:00:00 2001
From: Gustavo Saiani <gus@metabase.com>
Date: Thu, 1 Dec 2022 13:06:05 -0300
Subject: [PATCH] Build datetime filters as datetime before question is saved
(#26679)
---
frontend/src/metabase-lib/Dimension.ts | 4 ++
.../src/metabase-lib/expressions/config.js | 32 +++++-----
.../src/metabase-lib/expressions/resolver.js | 15 ++++-
.../expressions/typeinferencer.js | 6 +-
.../lib/expressions/resolver.unit.spec.js | 60 +++++++++++++++++++
.../expressions/typeinferencer.unit.spec.js | 15 +++--
6 files changed, 108 insertions(+), 24 deletions(-)
diff --git a/frontend/src/metabase-lib/Dimension.ts b/frontend/src/metabase-lib/Dimension.ts
index d481a677cef..d49fda38657 100644
--- a/frontend/src/metabase-lib/Dimension.ts
+++ b/frontend/src/metabase-lib/Dimension.ts
@@ -1264,6 +1264,10 @@ export class ExpressionDimension extends Dimension {
base_type = "type/Boolean";
break;
+ case MONOTYPE.DateTime:
+ base_type = "type/DateTime";
+ break;
+
// fallback
default:
base_type = "type/Float";
diff --git a/frontend/src/metabase-lib/expressions/config.js b/frontend/src/metabase-lib/expressions/config.js
index f3e40c7cff3..1d6cab352f2 100644
--- a/frontend/src/metabase-lib/expressions/config.js
+++ b/frontend/src/metabase-lib/expressions/config.js
@@ -324,69 +324,69 @@ export const MBQL_CLAUSES = {
"get-year": {
displayName: `year`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-quarter": {
displayName: `quarter`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-month": {
displayName: `month`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-week": {
displayName: `week`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
hasOptions: true, // optional mode parameter
},
"get-day": {
displayName: `day`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-day-of-week": {
displayName: `weekday`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-hour": {
displayName: `hour`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-minute": {
displayName: `minute`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"get-second": {
displayName: `second`,
type: "number",
- args: ["expression"],
+ args: ["datetime"],
},
"datetime-diff": {
displayName: `datetimeDiff`,
type: "number",
- args: ["expression", "expression", "string"],
+ args: ["datetime", "datetime", "string"],
requiresFeature: "datetime-diff",
},
"datetime-add": {
displayName: `datetimeAdd`,
- type: "expression",
- args: ["expression", "number", "string"],
+ type: "datetime",
+ args: ["datetime", "number", "string"],
},
"datetime-subtract": {
displayName: `datetimeSubtract`,
- type: "expression",
- args: ["expression", "number", "string"],
+ type: "datetime",
+ args: ["datetime", "number", "string"],
},
"convert-timezone": {
displayName: `convertTimezone`,
- type: "expression",
- args: ["expression", "string"],
+ type: "datetime",
+ args: ["datetime", "string"],
hasOptions: true,
requiresFeature: "convert-timezone",
},
diff --git a/frontend/src/metabase-lib/expressions/resolver.js b/frontend/src/metabase-lib/expressions/resolver.js
index 52c9ac8d780..2612db2c76d 100644
--- a/frontend/src/metabase-lib/expressions/resolver.js
+++ b/frontend/src/metabase-lib/expressions/resolver.js
@@ -38,11 +38,24 @@ function findMBQL(op) {
return clause;
}
+// a is the type of the argument expected,
+// as defined in MBQL_CLAUSES,
+// and b is the inferred type of the argument
const isCompatible = (a, b) => {
if (a === b) {
return true;
}
- if (a === "expression" && (b === "number" || b === "string")) {
+
+ // if b is a string, then it can be an arg to a function that expects a datetime argument.
+ // This allows datetime string literals to work as args for functions that expect datetime types.
+ // FIXME: By doing this we are allowing string columns to be arguments to functions, which isn’t valid MBQL.
+ if (a === "datetime" && b === "string") {
+ return true;
+ }
+ if (
+ a === "expression" &&
+ (b === "datetime" || b === "number" || b === "string")
+ ) {
return true;
}
if (a === "aggregation" && b === "number") {
diff --git a/frontend/src/metabase-lib/expressions/typeinferencer.js b/frontend/src/metabase-lib/expressions/typeinferencer.js
index 249c142b94b..5c0cb61caf5 100644
--- a/frontend/src/metabase-lib/expressions/typeinferencer.js
+++ b/frontend/src/metabase-lib/expressions/typeinferencer.js
@@ -6,6 +6,7 @@ export const MONOTYPE = {
Number: "number",
String: "string",
Boolean: "boolean",
+ DateTime: "datetime",
};
export function infer(mbql, env) {
@@ -37,9 +38,6 @@ export function infer(mbql, env) {
case "case":
return infer(mbql[1][0][1], env);
case "coalesce":
- case "convert-timezone":
- case "datetime-add":
- case "datetime-subtract":
return infer(mbql[1], env);
}
@@ -47,6 +45,8 @@ export function infer(mbql, env) {
if (func) {
const returnType = func.type;
switch (returnType) {
+ case "datetime":
+ return MONOTYPE.DateTime;
case "object":
return MONOTYPE.Undefined;
case "aggregation":
diff --git a/frontend/test/metabase/lib/expressions/resolver.unit.spec.js b/frontend/test/metabase/lib/expressions/resolver.unit.spec.js
index 74a6c14adc3..85ff4352508 100644
--- a/frontend/test/metabase/lib/expressions/resolver.unit.spec.js
+++ b/frontend/test/metabase/lib/expressions/resolver.unit.spec.js
@@ -159,6 +159,66 @@ describe("metabase-lib/expressions/resolve", () => {
it("should honor CONCAT's implicit casting", () => {
expect(() => expr(["concat", ["coalesce", "B", 1]])).not.toThrow();
});
+
+ describe("datetime functions", () => {
+ it("should resolve unchained functions", () => {
+ expect(() => expr(["get-week", "2022-01-01"])).not.toThrow();
+ expect(() =>
+ expr(["datetime-add", "2022-01-01", 1, "month"]),
+ ).not.toThrow();
+
+ // TODO: Implementation should be fine-tuned so that these throw
+ // as they are not really datetime
+ expect(() => expr(["get-day", A])).not.toThrow();
+ expect(() => expr(["get-day", "a"])).not.toThrow();
+ expect(() => expr(["get-day-of-week", A])).not.toThrow();
+ expect(() => expr(["get-day-of-week", "a"])).not.toThrow();
+ expect(() => expr(["get-week", A])).not.toThrow();
+ expect(() => expr(["get-week", "a"])).not.toThrow();
+ expect(() => expr(["get-month", A])).not.toThrow();
+ expect(() => expr(["get-month", "a"])).not.toThrow();
+ expect(() => expr(["get-quarter", A])).not.toThrow();
+ expect(() => expr(["get-quarter", "a"])).not.toThrow();
+ expect(() => expr(["get-year", A])).not.toThrow();
+ expect(() => expr(["get-year", "a"])).not.toThrow();
+ });
+
+ it("should resolve chained commmands", () => {
+ expect(() =>
+ expr([
+ "datetime-subtract",
+ ["datetime-add", "2022-01-01", 1, "month"],
+ 2,
+ "minute",
+ ]),
+ ).not.toThrow();
+ });
+
+ it("should chain datetime functions onto functions of compatible types", () => {
+ expect(() =>
+ expr([
+ "concat",
+ ["datetime-add", "2022-01-01", 1, "month"],
+ "a string",
+ ]),
+ ).not.toThrow();
+ });
+
+ it("should throw if chaining datetime functions onto functions of incompatible types", () => {
+ expect(() =>
+ expr(["trim", ["datetime-add", "2022-01-01", 1, "month"]]),
+ ).toThrow();
+ });
+
+ it("should throw if passing numbers as arguments expected to be datetime", () => {
+ expect(() => expr(["get-day", 15])).toThrow();
+ expect(() => expr(["get-day-of-week", 6])).toThrow();
+ expect(() => expr(["get-week", 52])).toThrow();
+ expect(() => expr(["get-month", 12])).toThrow();
+ expect(() => expr(["get-quarter", 3])).toThrow();
+ expect(() => expr(["get-year", 2025])).toThrow();
+ });
+ });
});
describe("for aggregations", () => {
diff --git a/frontend/test/metabase/lib/expressions/typeinferencer.unit.spec.js b/frontend/test/metabase/lib/expressions/typeinferencer.unit.spec.js
index ba145846e9c..a37a2c5fc03 100644
--- a/frontend/test/metabase/lib/expressions/typeinferencer.unit.spec.js
+++ b/frontend/test/metabase/lib/expressions/typeinferencer.unit.spec.js
@@ -116,12 +116,19 @@ describe("metabase-lib/expressions/typeinferencer", () => {
});
it("should infer the result of datetimeAdd, datetimeSubtract", () => {
- expect(type('datetimeAdd([CreatedAt], 2, "month")')).toEqual(
- "type/Datetime",
- );
+ expect(type('datetimeAdd([CreatedAt], 2, "month")')).toEqual("datetime");
+ expect(type('datetimeAdd("2022-01-01", 2, "month")')).toEqual("datetime");
expect(type('datetimeSubtract([CreatedAt], 2, "month")')).toEqual(
- "type/Datetime",
+ "datetime",
+ );
+ expect(type('datetimeSubtract("2022-01-01", 2, "month")')).toEqual(
+ "datetime",
);
+ expect(
+ type(
+ 'datetimeSubtract(datetimeAdd("2022-01-01", 2, "month"), 4, "minute")',
+ ),
+ ).toEqual("datetime");
});
it("should infer the result of datetimeExtract functions", () => {
--
GitLab