From 48a30f4b8ea64dd7770943de5c3b5cbe2267b74d Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Mon, 15 May 2023 22:22:49 +0300
Subject: [PATCH] Migrate default temporal bucket logic from Field.ts to ML
(#30745)
Fixes #30703.
---
.../metabase/shared/util/internal/time.clj | 7 ++
.../metabase/shared/util/internal/time.cljs | 7 ++
shared/src/metabase/shared/util/time.cljc | 1 +
src/metabase/lib/field.cljc | 34 ++++++++--
test/metabase/lib/field_test.cljc | 8 ++-
test/metabase/lib/metadata/jvm_test.clj | 31 +++++++++
test/metabase/lib/temporal_bucket_test.cljc | 64 +++++++++++++++++++
7 files changed, 144 insertions(+), 8 deletions(-)
diff --git a/shared/src/metabase/shared/util/internal/time.clj b/shared/src/metabase/shared/util/internal/time.clj
index 8f2622c77c7..e1446f59428 100644
--- a/shared/src/metabase/shared/util/internal/time.clj
+++ b/shared/src/metabase/shared/util/internal/time.clj
@@ -196,3 +196,10 @@
"Parses a time string that has been stripped of any time zone."
[value]
(t/local-time value))
+
+;;; ------------------------------------------------ arithmetic ------------------------------------------------------
+
+(defn day-diff
+ "Returns the time elapsed between `before` and `after` in days (an integer)."
+ [before after]
+ (.toDays (t/duration before after)))
diff --git a/shared/src/metabase/shared/util/internal/time.cljs b/shared/src/metabase/shared/util/internal/time.cljs
index 853aa0763ea..c446c564d84 100644
--- a/shared/src/metabase/shared/util/internal/time.cljs
+++ b/shared/src/metabase/shared/util/internal/time.cljs
@@ -148,3 +148,10 @@
"Parses a time string that has been stripped of any time zone."
[value]
(moment value parse-time-formats))
+
+;;; ------------------------------------------------ arithmetic ------------------------------------------------------
+
+(defn day-diff
+ "Returns the time elapsed between `before` and `after` in days."
+ [^moment/Moment before ^moment/Moment after]
+ (.diff after before "day"))
diff --git a/shared/src/metabase/shared/util/time.cljc b/shared/src/metabase/shared/util/time.cljc
index b5f4f558392..be684ee6f0e 100644
--- a/shared/src/metabase/shared/util/time.cljc
+++ b/shared/src/metabase/shared/util/time.cljc
@@ -14,6 +14,7 @@
(shared.ns/import-fn internal/same-day?)
(shared.ns/import-fn internal/same-month?)
(shared.ns/import-fn internal/same-year?)
+(shared.ns/import-fn internal/day-diff)
(defn- prep-options [options]
(merge internal/default-options (u/normalize-map options)))
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index a74c7981631..d26d6eb8475 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -20,6 +20,7 @@
[metabase.lib.temporal-bucket :as lib.temporal-bucket]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
+ [metabase.shared.util.time :as shared.ut]
[metabase.util :as u]
[metabase.util.humanization :as u.humanization]
[metabase.util.log :as log]
@@ -286,14 +287,35 @@
[query stage-number field-ref]
(lib.temporal-bucket/available-temporal-buckets query stage-number (resolve-field-metadata query stage-number field-ref)))
+(defn- fingerprint-based-default [fingerprint]
+ (u/ignore-exceptions
+ (when-let [{:keys [earliest latest]} (-> fingerprint :type :type/DateTime)]
+ (let [days (shared.ut/day-diff (shared.ut/coerce-to-timestamp earliest)
+ (shared.ut/coerce-to-timestamp latest))]
+ (when-not (NaN? days)
+ (condp > days
+ 1 :minute
+ 31 :day
+ 365 :week
+ :month))))))
+
+(defn- mark-default-unit [options unit]
+ (cond->> options
+ (some #(= (:unit %) unit) options)
+ (mapv (fn [option]
+ (cond-> (dissoc option :default)
+ (= (:unit option) unit) (assoc :default true))))))
+
(defmethod lib.temporal-bucket/available-temporal-buckets-method :metadata/field
[_query _stage-number field-metadata]
- (let [effective-type ((some-fn :effective-type :base-type) field-metadata)]
- (cond
- (isa? effective-type :type/DateTime) lib.temporal-bucket/datetime-bucket-options
- (isa? effective-type :type/Date) lib.temporal-bucket/date-bucket-options
- (isa? effective-type :type/Time) lib.temporal-bucket/time-bucket-options
- :else [])))
+ (let [effective-type ((some-fn :effective-type :base-type) field-metadata)
+ fingerprint-default (some-> field-metadata :fingerprint fingerprint-based-default)]
+ (cond-> (cond
+ (isa? effective-type :type/DateTime) lib.temporal-bucket/datetime-bucket-options
+ (isa? effective-type :type/Date) lib.temporal-bucket/date-bucket-options
+ (isa? effective-type :type/Time) lib.temporal-bucket/time-bucket-options
+ :else [])
+ fingerprint-default (mark-default-unit fingerprint-default))))
;;; ---------------------------------------- Binning ---------------------------------------------
(defmethod lib.binning/binning-method :field
diff --git a/test/metabase/lib/field_test.cljc b/test/metabase/lib/field_test.cljc
index 2bea9769d84..4a44022fc46 100644
--- a/test/metabase/lib/field_test.cljc
+++ b/test/metabase/lib/field_test.cljc
@@ -210,9 +210,13 @@
(deftest ^:parallel available-temporal-buckets-test
(doseq [{:keys [metadata expected-options]}
[{:metadata (get-in temporal-bucketing-mock-metadata [:fields :date])
- :expected-options lib.temporal-bucket/date-bucket-options}
+ :expected-options (-> lib.temporal-bucket/date-bucket-options
+ (update 0 dissoc :default)
+ (assoc-in [2 :default] true))}
{:metadata (get-in temporal-bucketing-mock-metadata [:fields :datetime])
- :expected-options lib.temporal-bucket/datetime-bucket-options}
+ :expected-options (-> lib.temporal-bucket/datetime-bucket-options
+ (update 2 dissoc :default)
+ (assoc-in [4 :default] true))}
{:metadata (get-in temporal-bucketing-mock-metadata [:fields :time])
:expected-options lib.temporal-bucket/time-bucket-options}]]
(testing (str (:base-type metadata) " Field")
diff --git a/test/metabase/lib/metadata/jvm_test.clj b/test/metabase/lib/metadata/jvm_test.clj
index 12029cc4d4d..cfff113f0ef 100644
--- a/test/metabase/lib/metadata/jvm_test.clj
+++ b/test/metabase/lib/metadata/jvm_test.clj
@@ -83,3 +83,34 @@
:display-name "Sum"
:source_alias "Orders"}]
(lib.metadata.calculation/metadata mlv2-query))))))
+
+(deftest ^:parallel temporal-bucketing-options-test
+ (mt/dataset sample-dataset
+ (let [query {:lib/type :mbql/query
+ :stages [{:lib/type :mbql.stage/mbql
+ :fields [[:field
+ {:lib/uuid (str (random-uuid))}
+ (mt/id :products :created_at)]]
+ :source-table (mt/id :products)}]
+ :database (mt/id)}
+ query (lib/query (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ query)]
+ (is (= [{:unit :minute}
+ {:unit :hour}
+ {:unit :day}
+ {:unit :week}
+ {:unit :month, :default true}
+ {:unit :quarter}
+ {:unit :year}
+ {:unit :minute-of-hour}
+ {:unit :hour-of-day}
+ {:unit :day-of-week}
+ {:unit :day-of-month}
+ {:unit :day-of-year}
+ {:unit :week-of-year}
+ {:unit :month-of-year}
+ {:unit :quarter-of-year}]
+ (->> (lib.metadata.calculation/metadata query)
+ first
+ (lib/available-temporal-buckets query)
+ (mapv #(select-keys % [:unit :default]))))))))
diff --git a/test/metabase/lib/temporal_bucket_test.cljc b/test/metabase/lib/temporal_bucket_test.cljc
index b7efd5a6fa4..937b71ba163 100644
--- a/test/metabase/lib/temporal_bucket_test.cljc
+++ b/test/metabase/lib/temporal_bucket_test.cljc
@@ -63,3 +63,67 @@
(is (= "Unknown unit"
(lib.temporal-bucket/describe-temporal-unit :unknown-unit)
(lib.temporal-bucket/describe-temporal-unit 2 :unknown-unit))))
+
+(deftest ^:parallel available-temporal-buckets-test
+ (let [column {:description nil
+ :lib/type :metadata/field
+ :database-is-auto-increment false
+ :fingerprint-version 5
+ :base-type :type/DateTimeWithLocalTZ
+ :semantic-type :type/CreationTimestamp
+ :database-required false
+ :table-id 806
+ :name "CREATED_AT"
+ :coercion-strategy nil
+ :lib/source :source/fields
+ :lib/source-column-alias "CREATED_AT"
+ :settings nil
+ :caveats nil
+ :nfc-path nil
+ :database-type "TIMESTAMP WITH TIME ZONE"
+ :effective-type :type/DateTimeWithLocalTZ
+ :fk-target-field-id nil
+ :custom-position 0
+ :active true
+ :id 3068
+ :parent-id nil
+ :points-of-interest nil
+ :visibility-type :normal
+ :lib/desired-column-alias "CREATED_AT"
+ :display-name "Created At"
+ :position 7
+ :has-field-values nil
+ :json-unfolding false
+ :preview-display true
+ :database-position 7
+ :fingerprint
+ {:global {:distinct-count 200, :nil% 0.0}}}
+ expected-units #{:minute :hour
+ :day :week :month :quarter :year
+ :minute-of-hour :hour-of-day
+ :day-of-week :day-of-month :day-of-year
+ :week-of-year :month-of-year :quarter-of-year}
+ expected-defaults [{:lib/type :type/temporal-bucketing-option, :unit :day, :default true}]]
+ (testing "missing fingerprint"
+ (let [column (dissoc column :fingerprint)
+ options (lib.temporal-bucket/available-temporal-buckets-method nil -1 column)]
+ (is (= expected-units
+ (into #{} (map :unit) options)))
+ (is (= expected-defaults
+ (filter :default options)))))
+ (testing "existing fingerprint"
+ (doseq [[latest unit] {"2019-04-15T13:34:19.931Z" :month
+ "2017-04-15T13:34:19.931Z" :week
+ "2016-05-15T13:34:19.931Z" :day
+ "2016-04-27T13:34:19.931Z" :minute
+ nil :day
+ "garbage" :day}]
+ (testing latest
+ (let [bounds {:earliest "2016-04-26T19:29:55.147Z"
+ :latest latest}
+ column (assoc-in column [:fingerprint :type :type/DateTime] bounds)
+ options (lib.temporal-bucket/available-temporal-buckets-method nil -1 column)]
+ (is (= expected-units
+ (into #{} (map :unit) options)))
+ (is (= (assoc-in expected-defaults [0 :unit] unit)
+ (filter :default options)))))))))
--
GitLab