Merge branch 'master' of github.com:metabase/metabase into collections

frontend/src/metabase/visualizations/PinMap.jsx

@@ -74,8 +74,8 @@ export default class PinMap extends Component {
         const latitudeIndex = _.findIndex(cols, (col) => col.name === settings["map.latitude_column"]);
         const longitudeIndex = _.findIndex(cols, (col) => col.name === settings["map.longitude_column"]);
         const points = rows.map(row => [
-            row[longitudeIndex],
-            row[latitudeIndex]
+            row[latitudeIndex],
+            row[longitudeIndex]
         ]);
         const bounds = L.latLngBounds(points);
         return { points, bounds };

frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js

@@ -278,6 +278,10 @@ function applyChartTooltips(chart, series, onHoverChange) {
                         { key: getFriendlyName(cols[index]), value: value, col: cols[index] }
                     ));
                 } else if (d.data) { // line, area, bar
+                    if (!isSingleSeriesBar) {
+                        let idx = determineSeriesIndexFromElement(this);
+                        cols = series[idx].data.cols;
+                    }
                     data = [
                         { key: getFriendlyName(cols[0]), value: d.data.key, col: cols[0] },
                         { key: getFriendlyName(cols[1]), value: d.data.value, col: cols[1] }
@@ -711,7 +715,8 @@ export default function lineAreaBar(element, { series, onHoverChange, onRender,
         if (isTimeseries) {
             // replace xValues with
             xValues = d3.time[xInterval.interval]
-                .range(xDomain[0], moment(xDomain[1]).add(1, "ms"), xInterval.count);
+                .range(xDomain[0], moment(xDomain[1]).add(1, "ms"), xInterval.count)
+                .map(d => moment(d));
             datas = fillMissingValues(
                 datas,
                 xValues,
@@ -802,7 +807,9 @@ export default function lineAreaBar(element, { series, onHoverChange, onRender,
     let yExtents = groups.map(group => d3.extent(group[0].all(), d => d.value));
     let yExtent = d3.extent([].concat(...yExtents));
 
-    if (!isScalarSeries && !isScatter && !isStacked && settings["graph.y_axis.auto_split"] !== false) {
+    // don't auto-split if the metric columns are all identical, i.e. it's a breakout multiseries
+    const hasDifferentYAxisColumns = _.uniq(series.map(s => s.data.cols[1])).length > 1;
+    if (!isScalarSeries && !isScatter && !isStacked && hasDifferentYAxisColumns && settings["graph.y_axis.auto_split"] !== false) {
         yAxisSplit = computeSplit(yExtents);
     } else {
         yAxisSplit = [series.map((s,i) => i)];

src/metabase/driver/bigquery.clj

@@ -93,6 +93,8 @@
    "INTEGER"   :type/Integer
    "RECORD"    :type/Dictionary ; RECORD -> field has a nested schema
    "STRING"    :type/Text
+   "DATE"      :type/Date
+   "DATETIME"  :type/DateTime
    "TIMESTAMP" :type/DateTime})
 
 (defn- table-schema->metabase-field-info [^TableSchema schema]
@@ -106,7 +108,7 @@
    :fields (set (table-schema->metabase-field-info (.getSchema (get-table database table-name))))})
 
 
-(def ^:private ^:const query-timeout-seconds 60)
+(def ^:private ^:const ^Integer query-timeout-seconds 60)
 
 (defn- ^QueryResponse execute-bigquery
   ([{{:keys [project-id]} :details, :as database} query-string]
@@ -142,6 +144,8 @@
    "INTEGER"   #(Long/parseLong %)
    "RECORD"    identity
    "STRING"    identity
+   "DATE"      parse-timestamp-str
+   "DATETIME"  parse-timestamp-str
    "TIMESTAMP" parse-timestamp-str})
 
 (defn- post-process-native
@@ -236,6 +240,9 @@
     :quarter-of-year (hx/quarter expr)
     :year            (hx/year expr)))
 
+(defn- date-string->literal [^String date-string]
+  (hx/->timestamp (hx/literal (u/format-date "yyyy-MM-dd 00:00" (u/->Date date-string)))))
+
 (defn- unix-timestamp->timestamp [expr seconds-or-milliseconds]
   (case seconds-or-milliseconds
     :seconds      (hsql/call :sec_to_timestamp  expr)
@@ -326,6 +333,12 @@
                     ag-type)))
     :else (str schema-name \. table-name \. field-name)))
 
+;; TODO - Making 2 DB calls for each field to fetch its dataset is inefficient and makes me cry, but this method is currently only used for SQL params so it's not a huge deal at this point
+(defn- field->identifier [{table-id :table_id, :as field}]
+  (let [db-id   (db/select-one-field :db_id 'Table :id table-id)
+        dataset (:dataset-id (db/select-one-field :details Database, :id db-id))]
+    (hsql/raw (apply format "[%s.%s.%s]" dataset (field/qualified-name-components field)))))
+
 ;; We have to override the default SQL implementations of breakout and order-by because BigQuery propogates casting functions in SELECT
 ;; BAD:
 ;; SELECT msec_to_timestamp([sad_toucan_incidents.incidents.timestamp]) AS [sad_toucan_incidents.incidents.timestamp], count(*) AS [count]
@@ -381,9 +394,11 @@
           :connection-details->spec  (constantly nil)                           ; since we don't use JDBC
           :current-datetime-fn       (constantly :%current_timestamp)
           :date                      (u/drop-first-arg date)
+          :date-string->literal      (u/drop-first-arg date-string->literal)
           :field->alias              (u/drop-first-arg field->alias)
+          :field->identifier         (u/drop-first-arg field->identifier)
           :prepare-value             (u/drop-first-arg prepare-value)
-          :quote-style               (constantly :sqlserver)                    ; we want identifiers quoted [like].[this]
+          :quote-style               (constantly :sqlserver)                    ; we want identifiers quoted [like].[this] initially (we have to convert them to [like.this] before executing)
           :string-length-fn          (u/drop-first-arg string-length-fn)
           :unix-timestamp->timestamp (u/drop-first-arg unix-timestamp->timestamp)})
 
@@ -419,9 +434,15 @@
           ;; people can manually specifiy "foreign key" relationships in admin and everything should work correctly.
           ;; Since we can't infer any "FK" relationships during sync our normal FK tests are not appropriate for BigQuery, so they're disabled for the time being.
           ;; TODO - either write BigQuery-speciifc tests for FK functionality or add additional code to manually set up these FK relationships for FK tables
-          :features              (constantly (when-not config/is-test?
-                                               ;; during unit tests don't treat bigquery as having FK support
-                                               #{:foreign-keys}))
+          :features              (constantly (set/union #{:basic-aggregations
+                                                          :standard-deviation-aggregations
+                                                          :native-parameters
+                                                          ;; Expression aggregations *would* work, but BigQuery doesn't support the auto-generated column names. BQ column names
+                                                          ;; can only be alphanumeric or underscores. If we slugified the auto-generated column names, we could enable this feature.
+                                                          #_:expression-aggregations}
+                                                        (when-not config/is-test?
+                                                          ;; during unit tests don't treat bigquery as having FK support
+                                                          #{:foreign-keys})))
           :field-values-lazy-seq (u/drop-first-arg field-values-lazy-seq)
           :mbql->native          (u/drop-first-arg mbql->native)}))
 

src/metabase/driver/generic_sql.clj

@@ -19,6 +19,7 @@
            java.util.Map
            (clojure.lang Keyword PersistentVector)
            com.mchange.v2.c3p0.ComboPooledDataSource
+           metabase.models.field.FieldInstance
            (metabase.query_processor.interface Field Value)))
 
 (defprotocol ISQLDriver
@@ -58,9 +59,20 @@
   (date [this, ^Keyword unit, field-or-value]
     "Return a HoneySQL form for truncating a date or timestamp field or value to a given resolution, or extracting a date component.")
 
+  (date-string->literal [this, ^String date-string]
+    "*OPTIONAL*. Return an appropriate HoneySQL form to represent a DATE-STRING literal.
+     The default implementation is just `hx/literal`; in other words, it just single-quotes DATE-STRING. Some drivers like BigQuery or Oracle need to do something more advanced.
+     (This is used for the implementation of SQL parameters).")
+
   (excluded-schemas ^java.util.Set [this]
     "*OPTIONAL*. Set of string names of schemas to skip syncing tables from.")
 
+  (field->identifier [this, ^FieldInstance field]
+    "*OPTIONAL*. Return a HoneySQL form that should be used as the identifier for FIELD.
+     The default implementation returns a keyword generated by from the components returned by `field/qualified-name-components`.
+     Other drivers like BigQuery need to do additional qualification, e.g. the dataset name as well.
+     (At the time of this writing, this is only used by the SQL parameters implementation; in the future it will probably be used in more places as well.)")
+
   (field-percent-urls [this field]
     "*OPTIONAL*. Implementation of the `:field-percent-urls-fn` to be passed to `make-analyze-table`.
      The default implementation is `fast-field-percent-urls`, which avoids a full table scan. Substitue this with `slow-field-percent-urls` for databases
@@ -203,6 +215,7 @@
   ([table field]
    (hx/qualify-and-escape-dots (:schema table) (:name table) (:name field))))
 
+
 (defn- query
   "Execute a HONEYSQL-FROM query against DATABASE, DRIVER, and optionally TABLE."
   ([driver database honeysql-form]
@@ -412,7 +425,9 @@
    :apply-page           (resolve 'metabase.driver.generic-sql.query-processor/apply-page)
    :column->special-type (constantly nil)
    :current-datetime-fn  (constantly :%now)
+   :date-string->literal (u/drop-first-arg hx/literal)
    :excluded-schemas     (constantly nil)
+   :field->identifier    (u/drop-first-arg (comp (partial apply hsql/qualify) field/qualified-name-components))
    :field->alias         (u/drop-first-arg name)
    :field-percent-urls   fast-field-percent-urls
    :prepare-value        (u/drop-first-arg :value)

src/metabase/driver/oracle.clj

@@ -86,6 +86,11 @@
                            3)
     :year            (hsql/call :extract :year v)))
 
+(defn- date-string->literal [^String date-string]
+  (hsql/call :to_timestamp
+    (hx/literal (u/format-date "yyyy-MM-dd" (u/->Date date-string)))
+    (hx/literal "YYYY-MM-DD")))
+
 (def ^:private ^:const now             (hsql/raw "SYSDATE"))
 (def ^:private ^:const date-1970-01-01 (hsql/call :to_timestamp (hx/literal :1970-01-01) (hx/literal :YYYY-MM-DD)))
 
@@ -218,6 +223,7 @@
           :connection-details->spec  (u/drop-first-arg connection-details->spec)
           :current-datetime-fn       (constantly now)
           :date                      (u/drop-first-arg date)
+          :date-string->literal      (u/drop-first-arg date-string->literal)
           :excluded-schemas          (fn [& _]
                                        (set/union
                                         #{"ANONYMOUS"

src/metabase/query_processor/macros.clj

@@ -58,11 +58,22 @@
   (when (metric? metric)
     (second metric)))
 
+(defn- maybe-unnest-ag-clause
+  "Unnest AG-CLAUSE if it's wrapped in a vector (i.e. if it is using the \"multiple-aggregation\" syntax).
+   (This is provided merely as a convenience to facilitate implementation of the Query Builder, so it can use the same UI for
+   normal aggregations and Metric creation. *METRICS DO NOT SUPPORT MULTIPLE AGGREGATIONS,* so if nested syntax is used, any
+   aggregation after the first will be ignored.)"
+  [ag-clause]
+  (if (and (coll? ag-clause)
+           (every? coll? ag-clause))
+    (first ag-clause)
+    ag-clause))
+
 (defn- expand-metric [metric-clause filter-clauses-atom]
   (let [{filter-clause :filter, ag-clause :aggregation} (db/select-one-field :definition 'Metric, :id (metric-id metric-clause))]
     (when filter-clause
       (swap! filter-clauses-atom conj filter-clause))
-    ag-clause))
+    (maybe-unnest-ag-clause ag-clause)))
 
 (defn- expand-metrics-in-ag-clause [query-dict filter-clauses-atom]
   (walk/postwalk (fn [form]

src/metabase/query_processor/sql_parameters.clj

@@ -46,21 +46,10 @@
   (first (hsql/format x
            :quoting ((resolve 'metabase.driver.generic-sql/quote-style) *driver*))))
 
-(defn- format-oracle-date [s]
-  (format "to_timestamp('%s', 'YYYY-MM-DD')" (u/format-date "yyyy-MM-dd" (u/->Date s))))
-
-(defn- oracle-driver? ^Boolean []
-  ;; we can't just import OracleDriver the normal way here because that would cause a cyclic load dependency
-  (boolean (when-let [oracle-driver-class (u/ignore-exceptions (Class/forName "metabase.driver.oracle.OracleDriver"))]
-             (instance? oracle-driver-class *driver*))))
-
-(defn- format-date
-  ;; This is a dirty dirty HACK! Unfortuantely Oracle is super-dumb when it comes to automatically converting strings to dates
-  ;; so we need to add the cast here
-  [date]
-  (if (oracle-driver?)
-    (format-oracle-date date)
-    (str \' date \')))
+(defn- format-date-string
+  "Format DATE-STRING as an appropriate literal using the driver's definition of `date-string->literal`."
+  ^String [^String date-string]
+  (honeysql->sql ((resolve 'metabase.driver.generic-sql/date-string->literal) *driver* date-string)))
 
 (extend-protocol ISQLParamSubstituion
   nil         (->sql [_]    "NULL")
@@ -73,20 +62,20 @@
 
   FieldInstance
   (->sql [this]
-    (->sql (let [identifier (apply hsql/qualify (field/qualified-name-components this))]
+    (->sql (let [identifier ((resolve 'metabase.driver.generic-sql/field->identifier) *driver* this)]
              (if (re-find #"^date/" (:type this))
                ((resolve 'metabase.driver.generic-sql/date) *driver* :day identifier)
                identifier))))
 
   Date
   (->sql [{:keys [s]}]
-    (format-date s))
+    (format-date-string s))
 
   DateRange
   (->sql [{:keys [start end]}]
     (if (= start end)
-      (format "= %s" (format-date start))
-      (format "BETWEEN %s AND %s" (format-date start) (format-date end))))
+      (format "= %s" (format-date-string start))
+      (format "BETWEEN %s AND %s" (format-date-string start) (format-date-string end))))
 
   Dimension
   (->sql [{:keys [field param], :as dimension}]
@@ -175,10 +164,10 @@
 
 (defn- parse-value-for-type [param-type value]
   (cond
-    (= param-type "number")                                (->NumberValue value)
+    (= param-type "number")                          (->NumberValue value)
     (and (= param-type "dimension")
-         (= (get-in value [:param :type]) "number"))       (update-in value [:param :value] ->NumberValue)
-    :else                                                  value))
+         (= (get-in value [:param :type]) "number")) (update-in value [:param :value] ->NumberValue)
+    :else                                            value))
 
 (defn- value-for-tag
   "Given a map TAG (a value in the `:template_tags` dictionary) return the corresponding value from the PARAMS sequence.

src/metabase/routes.clj

@@ -15,7 +15,7 @@
                                {:bootstrap_json (json/generate-string (public-settings/public-settings))})
         (slurp (io/resource "frontend_client/init.html")))
       resp/response
-      (resp/content-type "text/html")))
+      (resp/content-type "text/html; charset=utf-8")))
 
 ;; Redirect naughty users who try to visit a page other than setup if setup is not yet complete
 (defroutes ^{:doc "Top-level ring routes for Metabase."} routes

test/metabase/query_processor/macros_test.clj

@@ -4,9 +4,15 @@
             [metabase.models.metric :refer [Metric]]
             [metabase.models.segment :refer [Segment]]
             [metabase.models.table :refer [Table]]
-            [metabase.query-processor.macros :refer :all]
-            [metabase.test.data.users :refer :all]
-            [metabase.test.util :as tu]))
+            [metabase.query-processor :as qp]
+            (metabase.query-processor [expand :as ql]
+                                      [macros :refer :all])
+            [metabase.query-processor-test :refer :all]
+            [metabase.test.data :as data]
+            (metabase.test.data [datasets :as datasets]
+                                [users :refer :all])
+            [metabase.test.util :as tu]
+            [metabase.util :as u]))
 
 ;; expand-macros
 
@@ -127,3 +133,19 @@
                                :filter      ["AND" [">" 4 1] ["SEGMENT" segment-2-id]]
                                :breakout    [17]
                                :order_by    [[1 "ASC"]]}})))
+
+;; Check that a metric w/ multiple aggregation syntax (nested vector) still works correctly
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  [[2 118]
+   [3  39]
+   [4  24]]
+  (tu/with-temp Metric [metric {:table_id   (data/id :venues)
+                                :definition {:aggregation [[:sum [:field-id (data/id :venues :price)]]]
+                                             :filter      [:> [:field-id (data/id :venues :price)] 1]}}]
+    (format-rows-by [int int]
+      (rows (qp/process-query
+              {:database (data/id)
+               :type     :query
+               :query    {:source-table (data/id :venues)
+                          :aggregation  [["METRIC" (u/get-id metric)]]
+                          :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))

test/metabase/query_processor/sql_parameters_test.clj

@@ -12,7 +12,8 @@
             [metabase.test.data.datasets :as datasets]
             [metabase.test.data.generic-sql :as generic-sql]
             [metabase.test.util :as tu]
-            [metabase.test.data.generic-sql :as generic]))
+            [metabase.test.data.generic-sql :as generic]
+            [metabase.util :as u]))
 
 
 ;;; ------------------------------------------------------------ simple substitution -- {{x}} ------------------------------------------------------------
@@ -351,12 +352,15 @@
   (generic-sql/quote-name datasets/*driver* identifier))
 
 (defn- checkins-identifier []
-  (let [{table-name :name, schema :schema} (db/select-one ['Table :name :schema], :id (data/id :checkins))]
-    (str (when (seq schema)
-           (str (quote-name schema) \.))
-         (quote-name table-name))))
-
-;; as with the MBQL parameters tests redshift and crate fail for unknown reasons; disable their tests for now
+  ;; HACK ! I don't have all day to write protocol methods to make this work the "right" way so for BigQuery we will just hackily return the correct identifier here
+  (if (= datasets/*engine* :bigquery)
+    "[test_data.checkins]"
+    (let [{table-name :name, schema :schema} (db/select-one ['Table :name :schema], :id (data/id :checkins))]
+      (str (when (seq schema)
+             (str (quote-name schema) \.))
+           (quote-name table-name)))))
+
+;; as with the MBQL parameters tests Redshift and Crate fail for unknown reasons; disable their tests for now
 (def ^:private ^:const sql-parameters-engines
   (set/difference (engines-that-support :native-parameters) #{:redshift :crate}))
 

test/metabase/query_processor_test/expression_aggregations_test.clj

@@ -201,3 +201,39 @@
                :query    {:source-table (data/id :venues)
                           :aggregation  [:+ ["METRIC" (u/get-id metric)] 1]
                           :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))
+
+;; check that we can handle METRICS (ick) inside a NAMED clause
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  {:rows    [[2 118]
+             [3  39]
+             [4  24]]
+   :columns [(data/format-name "price")
+             (if (= *engine* :redshift) "my cool metric" "My Cool Metric")]}
+  (tu/with-temp Metric [metric {:table_id   (data/id :venues)
+                                :definition {:aggregation [:sum [:field-id (data/id :venues :price)]]
+                                             :filter      [:> [:field-id (data/id :venues :price)] 1]}}]
+    (format-rows-by [int int]
+      (rows+column-names (qp/process-query
+                           {:database (data/id)
+                            :type     :query
+                            :query    {:source-table (data/id :venues)
+                                       :aggregation  [[:named ["METRIC" (u/get-id metric)] "My Cool Metric"]]
+                                       :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))
+
+;; check that METRICS (ick) with a nested aggregation still work inside a NAMED clause
+(datasets/expect-with-engines (engines-that-support :expression-aggregations)
+  {:rows    [[2 118]
+             [3  39]
+             [4  24]]
+   :columns [(data/format-name "price")
+             (if (= *engine* :redshift) "my cool metric" "My Cool Metric")]}
+  (tu/with-temp Metric [metric {:table_id   (data/id :venues)
+                                :definition {:aggregation [[:sum [:field-id (data/id :venues :price)]]]
+                                             :filter      [:> [:field-id (data/id :venues :price)] 1]}}]
+    (format-rows-by [int int]
+      (rows+column-names (qp/process-query
+                           {:database (data/id)
+                            :type     :query
+                            :query    {:source-table (data/id :venues)
+                                       :aggregation  [[:named ["METRIC" (u/get-id metric)] "My Cool Metric"]]
+                                       :breakout     [(ql/breakout (ql/field-id (data/id :venues :price)))]}})))))