Adding Percent Metadata Type (#36145)

* Adding Percent Metadata Type

This adds the :type/Percentage metadata type to Metabase. It does
not currently do any fingerprinting. It just enables setting of the
type for the FE.

* Making :type/Percentage a semantic type as well as a decimal.

* support Percentage setting in the semantic type editor, infer percentage column formatting default

* Support for static-viz rendering when user metadata is `:type/Percentage`

This PR adds in user-defined metadata in `prep-for-html-rendering` so that `number-formatter` can properly render the column as a percentage. This may actually fix a family of issues as user-defined metadata doesn't appear to be used anywhere in this ns. We still need to consider cases where the formatting is specified by the viz settings, but this one step in the right direction.

* Percent semantic types render as percents in tables

This PR adds logic to properly capture a percent semantic type and renders it as such in a table static viz. Note that the `number-formatter` function in `metabase.pulse.render.common` has logic that renders the percent as a truncated int. IDK if this the desired long term behavior. I'll have to ask product that, but if we just make the change it could have potential impacts on places that expect this behavior. It seems like it would make more sense to be like Excel and allow each column to set its significant digits.

* Revert "support Percentage setting in the semantic type editor, infer percentage column formatting default"

This reverts commit e001e6312e6f0bd5700c1d9eb28c88829d87a70a.

---------

Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>

dev/src/dev/render_png.clj

@@ -2,24 +2,26 @@
   "Improve feedback loop for dealing with png rendering code. Will create images using the rendering that underpins
   pulses and subscriptions and open those images without needing to send them to slack or email."
   (:require
-   [clojure.java.io :as io]
-   [clojure.java.shell :as sh]
-   [hiccup.core :as hiccup]
-   [metabase.models.card :as card]
-   [metabase.models.user :as user]
-   [metabase.pulse :as pulse]
-   [metabase.pulse.render :as render]
-   [metabase.pulse.render.test-util :as render.tu]
-   [metabase.query-processor :as qp]
-   [metabase.query-processor.middleware.permissions :as qp.perms]))
+    [clojure.java.io :as io]
+    [clojure.java.shell :as sh]
+    [hiccup.core :as hiccup]
+    [metabase.models.card :as card]
+    [metabase.pulse :as pulse]
+    [metabase.pulse.render :as render]
+    [metabase.pulse.render.test-util :as render.tu]
+    [metabase.query-processor :as qp]
+    [toucan2.core :as t2])
+  (:import (java.io File)))
+
+(set! *warn-on-reflection* true)
 
 ;; taken from https://github.com/aysylu/loom/blob/master/src/loom/io.clj
 (defn- os
   "Returns :win, :mac, :unix, or nil"
   []
   (condp
-      #(<= 0 (.indexOf ^String %2 ^String %1))
-      (.toLowerCase (System/getProperty "os.name"))
+   #(<= 0 (.indexOf ^String %2 ^String %1))
+   (.toLowerCase (System/getProperty "os.name"))
     "win" :win
     "mac" :mac
     "nix" :unix
@@ -46,35 +48,44 @@
   match what's rendered on another system, like a docker container."
   [card-id]
   (let [{:keys [dataset_query] :as card} (t2/select-one card/Card :id card-id)
-        user                             (t2/select-one user/User)
-        query-results                    (binding [qp.perms/*card-id* nil]
-                                           (qp/process-query-and-save-execution!
-                                            (-> dataset_query
-                                                (assoc :async? false)
-                                                (assoc-in [:middleware :process-viz-settings?] true))
-                                            {:executed-by (:id user)
-                                             :context     :pulse
-                                             :card-id     card-id}))
+        query-results                    (qp/process-query dataset_query)
         png-bytes                        (render/render-pulse-card-to-png (pulse/defaulted-timezone card)
                                                                           card
                                                                           query-results
                                                                           1000)
-        tmp-file                         (java.io.File/createTempFile "card-png" ".png")]
+        tmp-file                         (File/createTempFile "card-png" ".png")]
     (with-open [w (java.io.FileOutputStream. tmp-file)]
       (.write w ^bytes png-bytes))
     (.deleteOnExit tmp-file)
     (open tmp-file)))
 
-(defn open-hiccup-as-html [hiccup]
+(defn render-pulse-card
+  "Render a pulse card as a data structure"
+  [card-id]
+  (let [{:keys [dataset_query] :as card} (t2/select-one card/Card :id card-id)
+        query-results (qp/process-query dataset_query)]
+    (render/render-pulse-card
+     :inline (pulse/defaulted-timezone card)
+     card
+     nil
+     query-results)))
+
+(defn open-hiccup-as-html
+  "Take a hiccup data structure, render it as html, then open it in the browser."
+  [hiccup]
   (let [html-str (hiccup/html hiccup)
-        tmp-file (java.io.File/createTempFile "card-html" ".html")]
-    (with-open [w (clojure.java.io/writer tmp-file)]
-      (.write w html-str))
+        tmp-file (File/createTempFile "card-html" ".html")]
+    (with-open [w (io/writer tmp-file)]
+      (.write w ^String html-str))
     (.deleteOnExit tmp-file)
     (open tmp-file)))
 
 (comment
   (render-card-to-png 1)
+
+  (let [{:keys [content]} (render-pulse-card 1)]
+    (open-hiccup-as-html content))
+
   ;; open viz in your browser
   (-> [["A" "B"]
        [1 2]
@@ -92,4 +103,3 @@
                                        :hidden-columns      {:hide [0 2]}})
       :viz-tree
       open-hiccup-as-html))
-

src/metabase/pulse/render.clj

@@ -119,7 +119,7 @@
   ((some-fn :include_csv :include_xls) card))
 
 (s/defn ^:private render-pulse-card-body :- common/RenderedPulseCard
-  [render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [data error], :as results}]
+  [render-type timezone-id :- (s/maybe s/Str) card dashcard {:keys [data error] :as results}]
   (try
     (when error
       (throw (ex-info (tru "Card has errors: {0}" error) (assoc results :card-error true))))

src/metabase/pulse/render/body.clj

@@ -151,8 +151,15 @@
 
 (s/defn ^:private query-results->row-seq
   "Returns a seq of stringified formatted rows that can be rendered into HTML"
-  [timezone-id :- (s/maybe s/Str) remapping-lookup cols rows viz-settings {:keys [bar-column min-value max-value]}]
-  (let [formatters (into [] (map #(get-format timezone-id % viz-settings)) cols)]
+  [timezone-id :- (s/maybe s/Str)
+   remapping-lookup
+   cols
+   rows
+   viz-settings
+   {:keys [bar-column min-value max-value]}]
+  (let [formatters (into []
+                         (map #(get-format timezone-id % viz-settings))
+                         cols)]
     (for [row rows]
       {:bar-width (some-> (and bar-column (bar-column row))
                           (normalize-bar-value min-value max-value))
@@ -171,15 +178,21 @@
   HTML"
   ([timezone-id :- (s/maybe s/Str) card data]
    (prep-for-html-rendering timezone-id card data {}))
-  ([timezone-id :- (s/maybe s/Str) card {:keys [cols rows viz-settings]}
+  ([timezone-id :- (s/maybe s/Str) {:keys [result_metadata] :as card} {:keys [cols rows viz-settings]}
     {:keys [bar-column] :as data-attributes}]
-   (let [remapping-lookup (create-remapping-lookup cols)]
+   (let [field-ref->curated-meta (zipmap (map :field_ref result_metadata) result_metadata)
+         ;; Add in user-curated metadata
+         cols (map (fn [{:keys [field_ref] :as col}] (into col (field-ref->curated-meta field_ref))) cols)
+         remapping-lookup (create-remapping-lookup cols)]
      (cons
       (query-results->header-row remapping-lookup card cols bar-column)
-      (query-results->row-seq timezone-id remapping-lookup cols
-                              (take rows-limit rows)
-                              viz-settings
-                              data-attributes)))))
+      (query-results->row-seq
+       timezone-id
+       remapping-lookup
+       cols
+       (take rows-limit rows)
+       viz-settings
+       data-attributes)))))
 
 (defn- strong-limit-text [number]
   [:strong {:style (style/style {:color style/color-gray-3})} (h (common/format-number number))])
@@ -204,7 +217,6 @@
                                 :margin-bottom :16px})}
      (trs "More results have been included as a file attachment")]))
 
-
 ;;; +----------------------------------------------------------------------------------------------------------------+
 ;;; |                                                     render                                                     |
 ;;; +----------------------------------------------------------------------------------------------------------------+
@@ -1001,7 +1013,6 @@
       [:img {:style (style/style {:display :block :width :100%})
              :src   (:image-src image-bundle)}]]}))
 
-
 (s/defmethod render :empty :- common/RenderedPulseCard
   [_ render-type _ _ _ _]
   (let [image-bundle (image-bundle/no-results-image-bundle render-type)]

src/metabase/pulse/render/common.clj

@@ -73,7 +73,9 @@
 (defn number-formatter
   "Return a function that will take a number and format it according to its column viz settings. Useful to compute the
   format string once and then apply it over many values."
-  [{:keys [effective_type base_type] col-id :id field-ref :field_ref col-name :name :as _column} viz-settings]
+  [{:keys [semantic_type effective_type base_type]
+    col-id :id field-ref :field_ref col-name :name :as _column}
+   viz-settings]
   (let [col-id (or col-id (second field-ref))
         column-settings (-> (get viz-settings ::mb.viz/column-settings)
                             (update-keys #(select-keys % [::mb.viz/field-id ::mb.viz/column-name])))
@@ -92,7 +94,7 @@
                                                                  (:type/Number global-settings)
                                                                  column-settings)
         integral?       (isa? (or effective_type base_type) :type/Integer)
-        percent?        (= number-style "percent")
+        percent?        (or (isa? semantic_type :type/Percentage) (= number-style "percent"))
         scientific?     (= number-style "scientific")
         [decimal grouping] (or number-separators
                                (get-in (public-settings/custom-formatting) [:type/Number :number_separators])
@@ -106,14 +108,13 @@
       (if (number? value)
         (let [scaled-value (* value (or scale 1))
               percent-scaled-value (* 100 scaled-value)
-              decimals-in-value (digits-after-decimal (if (= number-style "percent")
-                                                        (* 100 scaled-value)
-                                                        scaled-value))
+              decimals-in-value (digits-after-decimal (if percent? (* 100 scaled-value) scaled-value))
               decimal-digits (cond
                                decimals decimals ;; if user ever specifies # of decimals, use that
                                integral? 0
                                currency? (get-in currency/currency [(keyword (or currency "USD")) :decimal_digits])
                                (and percent? (> percent-scaled-value 100))   (min 2 decimals-in-value) ;; 5.5432 -> %554.32
+                               ;; This needs configuration. I don't see why we don't keep more significant digits.
                                (and percent? (> 100 percent-scaled-value 1)) (min 0 decimals-in-value) ;; 0.2555 -> %26
                                :else (if (>= scaled-value 1)
                                        (min 2 decimals-in-value) ;; values greater than 1 round to 2 decimal places
@@ -127,18 +128,18 @@
                         percent?    (str "%"))
               fmtr (doto (DecimalFormat. fmt-str symbols) (.setRoundingMode RoundingMode/HALF_UP))]
           (map->NumericWrapper
-            {:num-value value
-             :num-str   (str (when prefix prefix)
-                             (when (and currency? (or (nil? currency-style)
-                                                      (= currency-style "symbol")))
-                               (get-in currency/currency [(keyword (or currency "USD")) :symbol]))
-                             (when (and currency? (= currency-style "code"))
-                               (str (get-in currency/currency [(keyword (or currency "USD")) :code]) \space))
-                             (cond-> (.format fmtr scaled-value)
-                               (not decimals) (strip-trailing-zeroes decimal))
-                             (when (and currency? (= currency-style "name"))
-                               (str \space (get-in currency/currency [(keyword (or currency "USD")) :name_plural])))
-                             (when suffix suffix))}))
+           {:num-value value
+            :num-str   (str (when prefix prefix)
+                            (when (and currency? (or (nil? currency-style)
+                                                     (= currency-style "symbol")))
+                              (get-in currency/currency [(keyword (or currency "USD")) :symbol]))
+                            (when (and currency? (= currency-style "code"))
+                              (str (get-in currency/currency [(keyword (or currency "USD")) :code]) \space))
+                            (cond-> (.format fmtr scaled-value)
+                              (not decimals) (strip-trailing-zeroes decimal))
+                            (when (and currency? (= currency-style "name"))
+                              (str \space (get-in currency/currency [(keyword (or currency "USD")) :name_plural])))
+                            (when suffix suffix))}))
         value))))
 
 (s/defn format-number :- NumericWrapper

src/metabase/types.cljc

@@ -88,6 +88,11 @@
 (derive :type/Share :Semantic/*)
 (derive :type/Share :type/Float)
 
+;; A percent value (generally 0-100)
+
+(derive :type/Percentage :Semantic/*)
+(derive :type/Percentage :type/Decimal)
+
 ;; `:type/Currency` -- an actual currency data type, for example Postgres `money`.
 ;; `:type/Currency` -- a column that should be interpreted as money.
 ;;

test/metabase/pulse/render_test.clj

@@ -11,6 +11,8 @@
    [metabase.util :as u]
    [toucan2.tools.with-temp :as t2.with-temp]))
 
+(set! *warn-on-reflection* true)
+
 ;; Let's make sure rendering Pulses actually works
 
 (defn- render-pulse-card
@@ -157,3 +159,42 @@
                    DashboardCard dc1 {:dashboard_id (:id dashboard) :card_id (:id card)}]
       (binding [render/*include-description* true]
         (is (= "<h1>Card description</h1>\n" (last (:content (#'render/make-description-if-needed dc1 card)))))))))
+
+(deftest table-rendering-of-percent-types-test
+  (testing "If a column is marked as a :type/Percentage semantic type it should render as a percent"
+    (mt/dataset sample-dataset
+      (mt/with-temp [Card {base-card-id :id} {:dataset_query {:database (mt/id)
+                                                              :type     :query
+                                                              :query    {:source-table (mt/id :orders)
+                                                                         :expressions  {"Tax Rate" [:/
+                                                                                                    [:field (mt/id :orders :tax) {:base-type :type/Float}]
+                                                                                                    [:field (mt/id :orders :total) {:base-type :type/Float}]]},
+                                                                         :fields       [[:field (mt/id :orders :tax) {:base-type :type/Float}]
+                                                                                        [:field (mt/id :orders :total) {:base-type :type/Float}]
+                                                                                        [:expression "Tax Rate"]]
+                                                                         :limit        10}}}
+                     Card {:keys [dataset_query] :as card} {:dataset_query   {:type     :query
+                                                                              :database (mt/id)
+                                                                              :query    {:source-table (format "card__%s" base-card-id)}}
+                                                            :result_metadata [{:semantic_type :type/Percentage
+                                                                               :field_ref     [:field "Tax Rate" {:base-type :type/Float}]}]}]
+        ;; NOTE -- The logic in metabase.pulse.render.common/number-formatter renders values between 1 and 100 as an
+        ;; integer value. IDK if this is what we want long term, but this captures the current logic. If we do extend
+        ;; the significant digits in the formatter, we'll need to modify this test as well.
+        (let [query-results (qp/process-query dataset_query)
+              expected      (mapv (fn [row]
+                                    (format "%s%%" (Math/round ^float (* 100 (peek row)))))
+                                  (get-in query-results [:data :rows]))
+              rendered-card (render/render-pulse-card :inline (pulse/defaulted-timezone card) card nil query-results)
+              table         (-> rendered-card
+                                (get-in [:content 1 2 4 2])
+                                first
+                                second)
+              tax-col       (->>
+                              (rest (get-in table [2 1]))
+                              (map-indexed (fn [i v] [i (last v)]))
+                              (some (fn [[i v]] (when (= v "Tax Rate") i))))]
+          (testing "A column marked as semantic type :type/Percentage should be rendered with a percent sign"
+            (is (= expected
+                   (->> (get-in table [3 1])
+                        (map #(peek (get (vec (get % 2)) tax-col))))))))))))

test/metabase/sync/analyze/fingerprint_test.clj

@@ -59,7 +59,7 @@
             [:and
              [:< :fingerprint_version 2]
              [:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
-                               "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+                               "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
             [:and
              [:< :fingerprint_version 1]
              [:in :base_type #{"type/ImageURL" "type/AvatarURL"}]]]]}
@@ -81,7 +81,7 @@
               [:and
                [:< :fingerprint_version 2]
                [:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
-                                 "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+                                 "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
               ;; no type/Float stuff should be included for 1
               [:and
                [:< :fingerprint_version 1]
@@ -104,7 +104,7 @@
               [:and
                [:< :fingerprint_version 4]
                [:in :base_type #{"type/Decimal" "type/Latitude" "type/Longitude" "type/Coordinate" "type/Currency" "type/Float"
-                                 "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost"}]]
+                                 "type/Share" "type/Income" "type/Price" "type/Discount" "type/GrossMargin" "type/Cost" "type/Percentage"}]]
               [:and
                [:< :fingerprint_version 3]
                [:in :base_type #{"type/URL" "type/ImageURL" "type/AvatarURL"}]]