From 18b54e7a180c8e8716d095246c6efcdbb415fcef Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Tue, 22 Nov 2022 11:28:57 +0100
Subject: [PATCH] Refactor mbql schema for numeric-expressions and
date-time-expressions (#26635)
---
shared/src/metabase/mbql/schema.cljc | 105 +++++++-----------
.../query_processor/middleware/annotate.clj | 4 +-
2 files changed, 45 insertions(+), 64 deletions(-)
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index 549b7f213e7..88ca15da26a 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -127,8 +127,6 @@
(when (string? s)
(not= (.parse js/Date s) ##NaN))))
-;; TODO -- currently these are all the same between date/time/datetime
-
(def ^{:arglists '([s])} can-parse-date?
"Returns whether a string can be parsed to an ISO 8601 date or not."
#?(:clj (partial can-parse-iso-8601? DateTimeFormatter/ISO_DATE)
@@ -475,8 +473,8 @@
;; Expressions are "calculated column" definitions, defined once and then used elsewhere in the MBQL query.
-(def string-expressions
- "String functions"
+(def string-functions
+ "Functions that return string values. Should match [[StringExpression]]."
#{:substring :trim :rtrim :ltrim :upper :lower :replace :concat :regex-match-first :coalesce :case})
(declare StringExpression)
@@ -486,7 +484,7 @@
string?
s/Str
- (partial is-clause? string-expressions)
+ (partial is-clause? string-functions)
(s/recursive #'StringExpression)
(partial is-clause? :value)
@@ -495,34 +493,25 @@
:else
Field))
-;; TODO - rename to numeric-expressions
-(def arithmetic-expressions
- "Set of valid arithmetic expression clause keywords."
- #{:+ :- :/ :* :coalesce :length :round :ceil :floor :abs :power :sqrt :log :exp :case :datetime-diff})
+(def numeric-functions
+ "Functions that return numeric values. Should match [[NumericExpression]]."
+ #{:+ :- :/ :* :coalesce :length :round :ceil :floor :abs :power :sqrt :log :exp :case :datetime-diff
+ ;; extraction functions (get some component of a given temporal value/column)
+ :temporal-extract
+ ;; SUGAR drivers do not need to implement
+ :get-year :get-quarter :get-month :get-week :get-day :get-day-of-week :get-hour :get-minute :get-second})
-(def boolean-expressions
- "Set of valid boolean expression clause keywords."
+(def boolean-functions
+ "Functions that return boolean values. Should match [[BooleanExpression]]."
#{:and :or :not :< :<= :> :>= := :!=})
(def ^:private aggregations #{:sum :avg :stddev :var :median :percentile :min :max :cum-count :cum-sum :count-where :sum-where :share :distinct :metric :aggregation-options :count})
-;; TODO: expressions that return numerics should be in arithmetic-expressions
-(def temporal-extract-functions
- "Functions to extract components of a date, datetime."
- #{;; extraction functions (get some component of a given temporal value/column)
- :temporal-extract
- ;; SUGAR drivers do not need to implement
- :get-year :get-quarter :get-month :get-week :get-day :get-day-of-week :get-hour :get-minute :get-second})
-
-(def date-arithmetic-functions
- "Functions to do math with date, datetime."
+(def datetime-functions
+ "Functions that return Date or DateTime values. Should match [[DatetimeExpression]]."
#{:+ :datetime-add :datetime-subtract})
-(def date+time+timezone-functions
- "Date, time, and timezone related functions."
- (set/union temporal-extract-functions date-arithmetic-functions))
-
-(declare ArithmeticExpression)
+(declare NumericExpression)
(declare BooleanExpression)
(declare DatetimeExpression)
(declare Aggregation)
@@ -532,11 +521,8 @@
number?
s/Num
- (partial is-clause? arithmetic-expressions)
- (s/recursive #'ArithmeticExpression)
-
- (partial is-clause? temporal-extract-functions)
- (s/recursive #'DatetimeExpression)
+ (partial is-clause? numeric-functions)
+ (s/recursive #'NumericExpression)
(partial is-clause? aggregations)
(s/recursive #'Aggregation)
@@ -555,8 +541,7 @@
(partial is-clause? :value)
value
- ;; Recursively doing date math
- (partial is-clause? date-arithmetic-functions)
+ (partial is-clause? datetime-functions)
(s/recursive #'DatetimeExpression)
:else
@@ -570,21 +555,21 @@
boolean?
s/Bool
- (partial is-clause? boolean-expressions)
+ (partial is-clause? boolean-functions)
(s/recursive #'BooleanExpression)
- (partial is-clause? arithmetic-expressions)
- (s/recursive #'ArithmeticExpression)
+ (partial is-clause? numeric-functions)
+ (s/recursive #'NumericExpression)
+
+ (partial is-clause? datetime-functions)
+ (s/recursive #'DatetimeExpression)
string?
s/Str
- (partial is-clause? string-expressions)
+ (partial is-clause? string-functions)
(s/recursive #'StringExpression)
- (partial is-clause? temporal-extract-functions)
- (s/recursive #'DatetimeExpression)
-
(partial is-clause? :value)
value
@@ -663,13 +648,11 @@
(defclause ^{:requires-features #{:advanced-math-expressions}} log
x NumericExpressionArg)
-;; TODO: rename to NumericExpression*
-(declare ArithmeticExpression*)
+(declare NumericExpression*)
-;; TODO: rename to NumericExpression
-(def ^:private ArithmeticExpression
- "Schema for the definition of an arithmetic expression. All arithmetic expressions evaluate to numeric values."
- (s/recursive #'ArithmeticExpression*))
+(def ^:private NumericExpression
+ "Schema for the definition of a numeric expression. All numeric expressions evaluate to numeric values."
+ (s/recursive #'NumericExpression*))
;; The result is positive if x <= y, and negative otherwise.
;;
@@ -736,10 +719,7 @@
unit ArithmeticDateTimeUnit)
(def ^:private DatetimeExpression*
- (one-of + temporal-extract datetime-add datetime-subtract
- ;; SUGAR drivers do not need to implement
- get-year get-quarter get-month get-week get-day get-day-of-week
- get-hour get-minute get-second))
+ (one-of + datetime-add datetime-subtract))
(def DatetimeExpression
"Schema for the definition of a date function expression."
@@ -891,9 +871,9 @@
(def ^:private Filter*
(s/conditional
- (partial is-clause? arithmetic-expressions) ArithmeticExpression
- (partial is-clause? string-expressions) StringExpression
- (partial is-clause? boolean-expressions) BooleanExpression
+ (partial is-clause? numeric-functions) NumericExpression
+ (partial is-clause? string-functions) StringExpression
+ (partial is-clause? boolean-functions) BooleanExpression
:else
(one-of
;; filters drivers must implement
@@ -915,9 +895,10 @@
(defclause ^{:requires-features #{:basic-aggregations}} case
clauses CaseClauses, options (optional CaseOptions))
-;; TODO: rename to NumericExpression?
-(def ^:private ArithmeticExpression*
- (one-of + - / * coalesce length floor ceil round abs power sqrt exp log case datetime-diff))
+(def ^:private NumericExpression*
+ (one-of + - / * coalesce length floor ceil round abs power sqrt exp log case datetime-diff
+ temporal-extract get-year get-quarter get-month get-week get-day get-day-of-week
+ get-hour get-minute get-second))
(def ^:private StringExpression*
(one-of substring trim ltrim rtrim replace lower upper concat regex-match-first coalesce case))
@@ -927,10 +908,10 @@
"Schema for anything that is accepted as a top-level expression definition, either an arithmetic expression such as a
`:+` clause or a `:field` clause."
(s/conditional
- (partial is-clause? arithmetic-expressions) ArithmeticExpression
- (partial is-clause? string-expressions) StringExpression
- (partial is-clause? boolean-expressions) BooleanExpression
- (partial is-clause? date+time+timezone-functions) DatetimeExpression
+ (partial is-clause? numeric-functions) NumericExpression
+ (partial is-clause? string-functions) StringExpression
+ (partial is-clause? boolean-functions) BooleanExpression
+ (partial is-clause? datetime-functions) DatetimeExpression
(partial is-clause? :case) case
:else Field))
@@ -993,8 +974,8 @@
;; [:+ [:sum [:field 10 nil]] [:sum [:field 20 nil]]]
(def ^:private UnnamedAggregation*
- (s/if (partial is-clause? arithmetic-expressions)
- ArithmeticExpression
+ (s/if (partial is-clause? numeric-functions)
+ NumericExpression
(one-of avg cum-sum distinct stddev sum min max metric share count-where
sum-where case median percentile ag:var
;; SUGAR clauses
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index e42b98c3cbb..68ed8c8152a 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -178,10 +178,10 @@
(datetime-arithmetics? expression)
{:base_type :type/DateTime}
- (mbql.u/is-clause? mbql.s/string-expressions expression)
+ (mbql.u/is-clause? mbql.s/string-functions expression)
{:base_type :type/Text}
- (mbql.u/is-clause? mbql.s/arithmetic-expressions expression)
+ (mbql.u/is-clause? mbql.s/numeric-functions expression)
{:base_type :type/Float}
:else
--
GitLab