diff --git a/.circleci/config.yml b/.circleci/config.yml index 0d9a0c00ccfed09526d159cc66662fe4a30e19f1..76ab5a5a0fb3a70215c6937439c4cb4ceeba4b76 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -351,7 +351,7 @@ jobs: command: > /home/circleci/metabase/metabase/.circleci/skip-driver-tests.sh redshift || lein with-profile +ci test - no_output_timeout: 5m + no_output_timeout: 10m be-tests-presto: diff --git a/src/metabase/api/routes.clj b/src/metabase/api/routes.clj index 616d55cce3edaebd33f7493b1dd2ce317c32b4b1..27615f3a823f8114c90dea16f1be3c0a4dbd698c 100644 --- a/src/metabase/api/routes.clj +++ b/src/metabase/api/routes.clj @@ -4,7 +4,7 @@ [route :as route]] [metabase.api [activity :as activity] - [alert :as alert] + [alert :as alert] [automagic-dashboards :as magic] [card :as card] [collection :as collection] @@ -30,8 +30,8 @@ [setup :as setup] [slack :as slack] [table :as table] - [tiles :as tiles] [task :as task] + [tiles :as tiles] [user :as user] [util :as util]] [metabase.middleware :as middleware] diff --git a/src/metabase/api/util.clj b/src/metabase/api/util.clj index b6313460eb83c6e4405a061d82db2fcb03c099ac..16b74b33bd0a6d91aac13fa92c277b06b010b308 100644 --- a/src/metabase/api/util.clj +++ b/src/metabase/api/util.clj @@ -29,7 +29,7 @@ (stats/anonymous-usage-stats)) (api/defendpoint GET "/random_token" - "Return a cryptographically secure random 32-byte token, encoded as a hexidecimal string. + "Return a cryptographically secure random 32-byte token, encoded as a hexadecimal string. Intended for use when creating a value for `embedding-secret-key`." [] {:token (crypto-random/hex 32)}) diff --git a/src/metabase/driver/bigquery.clj b/src/metabase/driver/bigquery.clj index 993331dfd02d7e3f586e4aae2faee56f92df4b8b..2cffced368c0136298bce3382b7a07d93ce4ad03 100644 --- a/src/metabase/driver/bigquery.clj +++ b/src/metabase/driver/bigquery.clj @@ -22,7 +22,9 @@ [metabase.mbql [schema :as mbql.s] [util :as mbql.u]] - [metabase.models.table :as table] + [metabase.models + [field :refer [Field]] + [table :as table]] [metabase.query-processor [store :as qp.store] [util :as qputil]] @@ -317,7 +319,8 @@ ;; TODO - this is totally unnecessary now, we can just override `->honeysql` for `Field` and `Table` instead. FIXME! (defrecord ^:private BigQueryIdentifier [dataset-name ; optional; will use (dataset-name-for-current-query) otherwise table-name - field-name] + field-name + alias?] honeysql.format/ToSql (to-sql [{:keys [dataset-name table-name field-name], :as bq-id}] ;; Check to make sure the identifiers are valid and don't contain any sorts of escape characters since we are @@ -333,31 +336,21 @@ (assert (valid-bigquery-identifier? field-name) (tru "Invalid BigQuery identifier: ''{0}''" field-name))) ;; BigQuery identifiers should look like `dataset.table` or `dataset.table`.`field` (SAD!) - (str (format "`%s.%s`" (or dataset-name (dataset-name-for-current-query)) table-name) - (when (seq field-name) - (format ".`%s`" field-name))))) + (let [dataset-name (or dataset-name (dataset-name-for-current-query))] + (str + (if alias? + (format "`%s`" table-name) + (format "`%s.%s`" dataset-name table-name)) + (when (seq field-name) + (format ".`%s`" field-name)))))) (defn- honeysql-form->sql ^String [honeysql-form] {:pre [(map? honeysql-form)]} (let [[sql & args] (sql/honeysql-form->sql+args bq-driver honeysql-form)] (when (seq args) - (throw (Exception. (str (tru "BigQuery statements can't be parameterized!"))))) + (throw (Exception. (str (tru "BigQuery statements can''t be parameterized!"))))) sql)) -(defn- post-process-mbql [table-name {:keys [columns rows]}] - ;; Say we have an identifier like `veryNiceDataset.shakespeare`.`corpus`. We will alias it like - ;; `shakespeare___corpus` (because BigQuery does not let you include symbols in identifiers); during post-processing - ;; we can go ahead and strip off the table name from the alias since we don't want it to show up in the result - ;; column names - (let [demangle-name #(str/replace % (re-pattern (str \^ table-name "___")) "") - columns (map demangle-name columns) - rows (for [row rows] - (zipmap columns row)) - columns (vec (keys (first rows)))] - {:columns columns - :rows (for [row rows] - (mapv row columns))})) - ;; From the dox: Fields must contain only letters, numbers, and underscores, start with a letter or underscore, and be ;; at most 128 characters long. (defn- format-custom-field-name ^String [^String custom-field-name] @@ -407,34 +400,14 @@ [driver [_ field unit]] (sql/date driver unit (sqlqp/->honeysql driver field))) -(defmethod sqlqp/->honeysql [BigQueryDriver :field-id] - [_ [_ field-id]] - (let [{field-name :name, special-type :special_type, table-id :table_id} (qp.store/field field-id) - {table-name :name} (qp.store/table table-id) - field (map->BigQueryIdentifier - {:table-name table-name - :field-name field-name})] - (cond - (isa? special-type :type/UNIXTimestampSeconds) (unix-timestamp->timestamp field :seconds) - (isa? special-type :type/UNIXTimestampMilliseconds) (unix-timestamp->timestamp field :milliseconds) - :else field))) - -(defn- ag-ref->alias [[_ index]] - (let [{{aggregations :aggregation} :query} sqlqp/*query* - [ag-type :as ag] (nth aggregations index)] - (mbql.u/match-one ag - [:distinct _] "count" - [:expression operator & _] operator - [:named _ ag-name] ag-name - [ag-type & _] ag-type))) - -(defn- field->alias - "Generate an appropriate alias for a `field`. This will normally be something like `tableName___fieldName` (done this - way because BigQuery will not let us include symbols in identifiers, so we can't make our alias be - `tableName.fieldName`, like we do for other drivers)." - [driver {field-name :name, table-id :table_id, :as field}] - (let [{table-name :name} (qp.store/table table-id)] - (str table-name "___" field-name))) +(defmethod sqlqp/->honeysql [BigQueryDriver (class Field)] + [driver field] + (let [{table-name :name, :as table} (qp.store/table (:table_id field)) + field-identifier (map->BigQueryIdentifier + {:table-name table-name + :field-name (:name field) + :alias? (:alias? table)})] + (sqlqp/cast-unix-timestamp-field-if-needed driver field field-identifier))) (defn- field->identifier "Generate appropriate identifier for a Field for SQL parameters. (NOTE: THIS IS ONLY USED FOR SQL PARAMETERS!)" @@ -447,27 +420,18 @@ details (:details (qp.store/database))] (map->BigQueryIdentifier {:dataset-name (:dataset-id details), :table-name table-name, :field-name (:name field)}))) -(defn- field-clause->field [field-clause] - (when field-clause - (let [id-or-name (mbql.u/field-clause->id-or-literal field-clause)] - (when (integer? id-or-name) - (qp.store/field id-or-name))))) - -(defn- field->breakout-identifier [driver field-clause] - (let [alias (if (mbql.u/is-clause? :aggregation field-clause) - (ag-ref->alias field-clause) - (field->alias driver (field-clause->field field-clause)))] - (hsql/raw (str \` alias \`)))) - (defn- apply-breakout [driver honeysql-form {breakout-field-clauses :breakout, fields-field-clauses :fields}] (-> honeysql-form - ;; Group by all the breakout fields - ((partial apply h/group) (map #(field->breakout-identifier driver %) breakout-field-clauses)) + ;; Group by all the breakout fields. + ;; + ;; Unlike other SQL drivers, BigQuery requires that we refer to Fields using the alias we gave them in the + ;; `SELECT` clause, rather than repeating their definitions. + ((partial apply h/group) (map (partial sqlqp/field-clause->alias driver) breakout-field-clauses)) ;; Add fields form only for fields that weren't specified in :fields clause -- we don't want to include it ;; twice, or HoneySQL will barf ((partial apply h/merge-select) (for [field-clause breakout-field-clauses :when (not (contains? (set fields-field-clauses) field-clause))] - (sqlqp/as driver (sqlqp/->honeysql driver field-clause) field-clause))))) + (sqlqp/as driver field-clause))))) (defn apply-source-table "Copy of the Generic SQL implementation of `apply-source-table` that prepends the current dataset ID to the table @@ -488,22 +452,34 @@ honeysql-form (h/merge-left-join honeysql-form [(map->BigQueryIdentifier {:table-name table-name}) - (map->BigQueryIdentifier {:table-name join-alias})] + (map->BigQueryIdentifier {:table-name join-alias, :alias? true})] [:= (map->BigQueryIdentifier {:table-name source-table-name, :field-name (:name source-field)}) - (map->BigQueryIdentifier {:table-name join-alias, :field-name (:name pk-field)})])] + (map->BigQueryIdentifier {:table-name join-alias, :field-name (:name pk-field), :alias? true})])] (if (seq more) (recur honeysql-form more) honeysql-form))))) +(defn- ag-ref->alias [[_ index]] + (let [{{aggregations :aggregation} :query} sqlqp/*query* + [ag-type :as ag] (nth aggregations index)] + (mbql.u/match-one ag + [:distinct _] :count + [:expression operator & _] operator + [:named _ ag-name] (keyword ag-name) + [ag-type & _] ag-type))) + (defn- apply-order-by [driver honeysql-form {subclauses :order-by, :as query}] (loop [honeysql-form honeysql-form, [[direction field-clause] & more] subclauses] - (let [honeysql-form (h/merge-order-by honeysql-form [(field->breakout-identifier driver field-clause) + (let [honeysql-form (h/merge-order-by honeysql-form [(if (mbql.u/is-clause? :aggregation field-clause) + (ag-ref->alias field-clause) + (sqlqp/field-clause->alias driver field-clause)) direction])] (if (seq more) (recur honeysql-form more) honeysql-form)))) + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Other Driver / SQLDriver Method Implementations | ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -521,19 +497,21 @@ * Runs our customs `honeysql-form->sql` method * Incldues `table-name` in the resulting map (do not remember why we are doing so, perhaps it is needed to run the query)" - [{database-id :database - {source-table-id :source-table} :query - :as outer-query}] + [{database-id :database + {source-table-id :source-table, source-query :source-query} :query + :as outer-query}] {:pre [(integer? database-id)]} (let [dataset-id (-> (qp.store/database) :details :dataset-id) aliased-query (pre-alias-aggregations outer-query) - {table-name :name} (qp.store/table source-table-id)] + {table-name :name} (some-> source-table-id qp.store/table)] (assert (seq dataset-id)) (binding [sqlqp/*query* (assoc aliased-query :dataset-id dataset-id)] {:query (->> aliased-query (sqlqp/build-honeysql-form bq-driver) honeysql-form->sql) - :table-name table-name + :table-name (or table-name + (when source-query + sqlqp/source-query-alias)) :mbql? true}))) (defn- effective-query-timezone [database] @@ -546,12 +524,10 @@ :as outer-query}] (let [database (qp.store/database)] (binding [*bigquery-timezone* (effective-query-timezone database)] - (let [sql (str "-- " (qputil/query->remark outer-query) "\n" (if (seq params) - (unprepare/unprepare (cons sql params)) - sql)) - results (process-native* database sql)] - (cond->> results - mbql? (post-process-mbql table-name)))))) + (let [sql (str "-- " (qputil/query->remark outer-query) "\n" (if (seq params) + (unprepare/unprepare (cons sql params)) + sql))] + (process-native* database sql))))) ;; BigQuery doesn't return a timezone with it's time strings as it's always UTC, JodaTime parsing also defaults to UTC @@ -570,7 +546,6 @@ :connection-details->spec (constantly nil) :current-datetime-fn (constantly :%current_timestamp) :date (u/drop-first-arg date) - :field->alias field->alias :field->identifier (u/drop-first-arg field->identifier) :quote-style (constantly :mysql) :string-length-fn (u/drop-first-arg string-length-fn) @@ -618,6 +593,7 @@ :native-parameters :expression-aggregations :binning + :nested-queries :native-query-params} (when-not config/is-test? ;; during unit tests don't treat bigquery as having FK diff --git a/src/metabase/driver/generic_sql/query_processor.clj b/src/metabase/driver/generic_sql/query_processor.clj index 4092e3ab980af39490546aedd13f2a2555019f69..395f2fc15e50c5f8c55eac3c67f0307c1f13e7c9 100644 --- a/src/metabase/driver/generic_sql/query_processor.clj +++ b/src/metabase/driver/generic_sql/query_processor.clj @@ -40,34 +40,6 @@ Each nested query increments this counter by 1." 0) -;;; +----------------------------------------------------------------------------------------------------------------+ -;;; | Other Formatting | -;;; +----------------------------------------------------------------------------------------------------------------+ - - -(s/defn ^:private qualified-alias - "Convert the given `FIELD` to a stringified alias, for use in a SQL `AS` clause." - [driver, field :- (class Field)] - (some->> field - (sql/field->alias driver) - hx/qualify-and-escape-dots)) - -(defn as - "Generate a FORM `AS` FIELD alias using the name information of FIELD." - [driver form field-clause] - (let [expression-name (when (mbql.u/is-clause? :expression field-clause) - (second field-clause)) - field (when-not expression-name - (let [id-or-name (mbql.u/field-clause->id-or-literal field-clause)] - (when (integer? id-or-name) - (qp.store/field id-or-name))))] - (if-let [alias (cond - expression-name expression-name - field (qualified-alias driver field))] - [form alias] - form))) - - ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | ->honeysql multimethod def & low-level method impls | ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -213,6 +185,52 @@ (driver/date-interval driver unit amount)))) +;;; +----------------------------------------------------------------------------------------------------------------+ +;;; | Field Aliases (AS Forms) | +;;; +----------------------------------------------------------------------------------------------------------------+ + +(s/defn ^:private qualified-alias :- s/Keyword + "Convert the given `field` to a stringified alias, for use in a SQL `AS` clause." + [driver, field :- (class Field)] + (some->> field + (sql/field->alias driver) + hx/qualify-and-escape-dots)) + +(s/defn field-clause->alias :- s/Keyword + "Generate an approriate alias (e.g., for use with SQL `AN`) for a Field clause of any type." + [driver, field-clause :- mbql.s/Field] + (let [expression-name (when (mbql.u/is-clause? :expression field-clause) + (second field-clause)) + id-or-name (when-not expression-name + (mbql.u/field-clause->id-or-literal field-clause)) + field (when (integer? id-or-name) + (qp.store/field id-or-name))] + (cond + expression-name (keyword (hx/escape-dots expression-name)) + field (qualified-alias driver field) + (string? id-or-name) (keyword (hx/escape-dots id-or-name))))) + +(defn as + "Generate HoneySQL for an `AS` form (e.g. `<form> AS <field>`) using the name information of a `field-clause`. The + HoneySQL representation of on `AS` clause is a tuple like `[<form> <alias>]`. + + In some cases where the alias would be redundant, such as unwrapped field literals, this returns the form as-is. + + (as [:field-literal \"x\" :type/Text]) + ;; -> <compiled-form> + ;; -> SELECT \"x\" + + (as [:datetime-field [:field-literal \"x\" :type/Text] :month]) + ;; -> [<compiled-form> :x] + ;; -> SELECT date_extract(\"x\", 'month') AS \"x\"" + ([driver field-clause] + (as driver (->honeysql driver field-clause) field-clause)) + ([driver form field-clause] + (if (mbql.u/is-clause? :field-literal field-clause) + form + [form (field-clause->alias driver field-clause)]))) + + ;;; +----------------------------------------------------------------------------------------------------------------+ ;;; | Clause Handlers | ;;; +----------------------------------------------------------------------------------------------------------------+ @@ -239,14 +257,14 @@ (as-> honeysql-form new-hsql (apply h/merge-select new-hsql (for [field-clause breakout-fields :when (not (contains? (set fields-fields) field-clause))] - (as driver (->honeysql driver field-clause) field-clause))) + (as driver field-clause))) (apply h/group new-hsql (map (partial ->honeysql driver) breakout-fields)))) (defn apply-fields "Apply a `fields` clause to HONEYSQL-FORM. Default implementation of `apply-fields` for SQL drivers." [driver honeysql-form {fields :fields}] (apply h/merge-select honeysql-form (for [field fields] - (as driver (->honeysql driver field) field)))) + (as driver field)))) ;;; ----------------------------------------------------- filter ----------------------------------------------------- @@ -425,7 +443,12 @@ ;; TODO - it seems to me like we could actually properly handle nested nested queries by giving each level of nesting ;; a different alias -(def ^:private source-query-alias :source) +(def source-query-alias + "Alias to use for source queries, e.g.: + + SELECT source.* + FROM ( SELECT * FROM some_table ) source" + :source) (defn- apply-source-query "Handle a `:source-query` clause by adding a recursive `SELECT` or native query. At the time of this writing, all diff --git a/src/metabase/driver/sparksql.clj b/src/metabase/driver/sparksql.clj index a429e0a53ce698b6c0cf2391f470913ff4738fc9..67a2bdb0c8d15557f717718597efa13ace895df5 100644 --- a/src/metabase/driver/sparksql.clj +++ b/src/metabase/driver/sparksql.clj @@ -15,7 +15,6 @@ [generic-sql :as sql] [hive-like :as hive-like]] [metabase.driver.generic-sql.query-processor :as sqlqp] - [metabase.mbql.util :as mbql.u] [metabase.models.field :refer [Field]] [metabase.query-processor [store :as qp.store] @@ -34,54 +33,18 @@ ;;; ------------------------------------------ Custom HoneySQL Clause Impls ------------------------------------------ -(def ^:private source-table-alias "t1") - -(defn- resolve-table-alias [{field-name :name, special-type :special_type, table-id :table_id, :as field}] - (let [{schema-name :schema, table-name :name} (qp.store/table table-id) - source-table (qp.store/table (mbql.u/query->source-table-id sqlqp/*query*)) - matching-join-table (some (fn [{:keys [table-id]}] - (let [join-table (qp.store/table table-id)] - (when (and (= schema-name (:schema join-table)) - (= table-name (:name join-table))) - join-table))) - (get-in sqlqp/*query* [:query :join-tables]))] - (cond - (and (= schema-name (:schema source-table)) - (= table-name (:name source-table))) - (assoc field :schema nil, :table source-table-alias) - - matching-join-table - (assoc field :schema nil, :table (:join-alias matching-join-table)) - - :else - field))) - -(defmethod sqlqp/->honeysql [SparkSQLDriver (class Field)] - [driver field-before-aliasing] - (let [{schema-name :schema - table-name :table - special-type :special_type - field-name :name} (resolve-table-alias field-before-aliasing) - field (keyword (hx/qualify-and-escape-dots schema-name table-name field-name))] - (cond - (isa? special-type :type/UNIXTimestampSeconds) (sql/unix-timestamp->timestamp driver field :seconds) - (isa? special-type :type/UNIXTimestampMilliseconds) (sql/unix-timestamp->timestamp driver field :milliseconds) - :else field))) - -(defn- apply-join-tables - [honeysql-form {join-tables :join-tables}] - (loop [honeysql-form honeysql-form, [{:keys [table-id pk-field-id fk-field-id schema join-alias]} & more] join-tables] - (let [{table-name :name} (qp.store/table table-id) - source-field (qp.store/field fk-field-id) - pk-field (qp.store/field pk-field-id) - honeysql-form (h/merge-left-join honeysql-form - [(hx/qualify-and-escape-dots schema table-name) (keyword join-alias)] - [:= - (hx/qualify-and-escape-dots source-table-alias (:field-name source-field)) - (hx/qualify-and-escape-dots join-alias (:field-name pk-field))])] - (if (seq more) - (recur honeysql-form more) - honeysql-form)))) +(def ^:private source-table-alias + "Default alias for all source tables. (Not for source queries; those still use the default SQL QP alias of `source`.)" + "t1") + +(defmethod sqlqp/->honeysql [SparkSQLDriver (class Field)] + [driver field] + (let [table (qp.store/table (:table_id field)) + table-name (if (:alias? table) + (:name table) + source-table-alias) + field-identifier (keyword (hx/qualify-and-escape-dots table-name (:name field)))] + (sqlqp/cast-unix-timestamp-field-if-needed driver field field-identifier))) (defn- apply-page-using-row-number-for-offset "Apply `page` clause to HONEYSQL-FROM, using row_number() for drivers that do not support offsets" @@ -203,7 +166,6 @@ (merge (sql/ISQLDriverDefaultsMixin) {:apply-page (u/drop-first-arg apply-page-using-row-number-for-offset) :apply-source-table (u/drop-first-arg apply-source-table) - :apply-join-tables (u/drop-first-arg apply-join-tables) :column->base-type (u/drop-first-arg hive-like/column->base-type) :connection-details->spec (u/drop-first-arg connection-details->spec) :date (u/drop-first-arg hive-like/date) diff --git a/src/metabase/email/messages.clj b/src/metabase/email/messages.clj index da49274b311abd4d44860dd0168c332b88fd6b4b..59c746b67cfe93b8e54eab1f34dc7de6158fb2eb 100644 --- a/src/metabase/email/messages.clj +++ b/src/metabase/email/messages.clj @@ -14,15 +14,14 @@ [metabase.util [date :as du] [export :as export] - [i18n :refer [tru]] + [i18n :refer [trs tru]] [quotation :as quotation] [urls :as url]] [stencil [core :as stencil] [loader :as stencil-loader]] [toucan.db :as db]) - (:import [java.io File FileOutputStream IOException] - java.util.Arrays)) + (:import [java.io File IOException])) ;; Dev only -- disable template caching (when config/is-dev? @@ -36,19 +35,21 @@ {:quotation (:quote data-quote) :quotationAuthor (:author data-quote)})) -(def ^:private ^:const notification-context +(def ^:private notification-context {:emailType "notification" :logoHeader true}) -(def ^:private ^:const abandonment-context - {:heading "We’d love your feedback." - :callToAction "It looks like Metabase wasn’t quite a match for you. Would you mind taking a fast 5 question survey to help the Metabase team understand why and make things better in the future?" - :link "http://www.metabase.com/feedback/inactive"}) +(defn- abandonment-context [] + {:heading (str (trs "We’d love your feedback.")) + :callToAction (str (trs "It looks like Metabase wasn’t quite a match for you.") + " " + (trs "Would you mind taking a fast 5 question survey to help the Metabase team understand why and make things better in the future?")) + :link "https://www.metabase.com/feedback/inactive"}) -(def ^:private ^:const follow-up-context - {:heading "We hope you've been enjoying Metabase." - :callToAction "Would you mind taking a fast 6 question survey to tell us how it’s going?" - :link "http://www.metabase.com/feedback/active"}) +(defn- follow-up-context [] + {:heading (str (trs "We hope you've been enjoying Metabase.")) + :callToAction (str (trs "Would you mind taking a fast 6 question survey to tell us how it’s going?")) + :link "https://www.metabase.com/feedback/active"}) ;;; ### Public Interface @@ -75,8 +76,9 @@ (defn- all-admin-recipients "Return a sequence of email addresses for all Admin users. - The first recipient will be the site admin (or oldest admin if unset), which is the address that should be used in `mailto` links - (e.g., for the new user to email with any questions)." + + The first recipient will be the site admin (or oldest admin if unset), which is the address that should be used in + `mailto` links (e.g., for the new user to email with any questions)." [] (concat (when-let [admin-email (public-settings/admin-email)] [admin-email]) @@ -124,7 +126,8 @@ :message-type :html :message message-body))) -;; TODO - I didn't write these function and I don't know what it's for / what it's supposed to be doing. If this is determined add appropriate documentation +;; TODO - I didn't write these function and I don't know what it's for / what it's supposed to be doing. If this is +;; determined add appropriate documentation (defn- model-name->url-fn [model] (case model @@ -172,8 +175,8 @@ context (merge notification-context (random-quote-context) (if (= "abandon" msg-type) - abandonment-context - follow-up-context)) + (abandonment-context) + (follow-up-context))) message-body (stencil/render-file "metabase/email/follow_up_email" context)] (email/send-message! :subject subject diff --git a/src/metabase/mbql/util.clj b/src/metabase/mbql/util.clj index d6fac37a89167abc013947a23bfb8bac474f7c12..9489c0be21aa6dbec8e54a799da76c0461d217e5 100644 --- a/src/metabase/mbql/util.clj +++ b/src/metabase/mbql/util.clj @@ -243,8 +243,11 @@ (s/defn query->source-table-id :- (s/maybe su/IntGreaterThanZero) "Return the source Table ID associated with `query`, if applicable; handles nested queries as well. If `query` is - `nil`, returns `nil`." - {:argslists '([outer-query])} + `nil`, returns `nil`. + + Throws an Exception when it encounters a unresolved source query (i.e., the `:source-table \"card__id\"` + form), because it cannot return an accurate result for a query that has not yet been preprocessed." + {:arglists '([outer-query])} [{{source-table-id :source-table, source-query :source-query} :query, query-type :type, :as query}] (cond ;; for native queries, there's no source table to resolve @@ -265,7 +268,7 @@ (throw (Exception. (str - (tru "Error: query's source query has not been resolved. You probably need to `preprocess` the query first.")))) + (tru "Error: query''s source query has not been resolved. You probably need to `preprocess` the query first.")))) ;; otherwise resolve the source Table :else diff --git a/src/metabase/models/params.clj b/src/metabase/models/params.clj index ce25a08cb156044404b7c2551ff12715996f4c68..dd36ef34d2b6dc195d2c030b8e086abffb9f6c5a 100644 --- a/src/metabase/models/params.clj +++ b/src/metabase/models/params.clj @@ -5,6 +5,7 @@ [db :as mdb] [util :as u]] [metabase.mbql.util :as mbql.u] + [metabase.util.i18n :as ui18n :refer [trs]] [toucan [db :as db] [hydrate :refer [hydrate]]])) @@ -29,7 +30,7 @@ field-form :else - (throw (IllegalArgumentException. (str "Don't know what to do with: " field-form))))) + (throw (IllegalArgumentException. (str (trs "Don't know what to do with:") " " field-form))))) (defn wrap-field-id-if-needed "Wrap a raw Field ID in a `:field-id` clause if needed." @@ -42,7 +43,7 @@ [:field-id field-id-or-form] :else - (throw (IllegalArgumentException. (str "Don't know how to wrap:" field-id-or-form))))) + (throw (IllegalArgumentException. (str (trs "Don't know how to wrap:") " " field-id-or-form))))) (defn- field-ids->param-field-values "Given a collection of PARAM-FIELD-IDS return a map of FieldValues for the Fields they reference. diff --git a/src/metabase/models/permissions.clj b/src/metabase/models/permissions.clj index d086491ee802480342093514f6f24aa4c0217573..76ad328003c60944fabe203c5ec49ced692aad3a 100644 --- a/src/metabase/models/permissions.clj +++ b/src/metabase/models/permissions.clj @@ -559,6 +559,7 @@ [old-graph new-graph] (when (not= (:revision old-graph) (:revision new-graph)) (throw (ui18n/ex-info (str (tru "Looks like someone else edited the permissions and your data is out of date.") + " " (tru "Please fetch new data and try again.")) {:status-code 409})))) diff --git a/src/metabase/models/user.clj b/src/metabase/models/user.clj index 6f2dd9a005130d342700741ba7bbf3ce56e622eb..a723f9ee394f24431e89e10d8def59dbec80b29b 100644 --- a/src/metabase/models/user.clj +++ b/src/metabase/models/user.clj @@ -181,7 +181,7 @@ (s/defn create-new-google-auth-user! "Convenience for creating a new user via Google Auth. This account is considered active immediately; thus all active - admins will recieve an email right away." + admins will receive an email right away." [new-user :- NewUser] (u/prog1 (insert-new-user! (assoc new-user :google_auth true)) ;; send an email to everyone including the site admin if that's set @@ -189,7 +189,7 @@ (s/defn create-new-ldap-auth-user! "Convenience for creating a new user via LDAP. This account is considered active immediately; thus all active admins - will recieve an email right away." + will receive an email right away." [new-user :- NewUser] (insert-new-user! (-> new-user ;; We should not store LDAP passwords diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj index ea26d5225e80901aa428a8ba529f09c880adbcaf..2bcf23a0097d2b7ba7e24ab664aa5f89a6c58c28 100644 --- a/src/metabase/query_processor.clj +++ b/src/metabase/query_processor.clj @@ -169,7 +169,7 @@ ;; ...which ends up getting caught by the `catch-exceptions` middleware. Add a final post-processing function ;; around that which will return whatever we delivered into the `:results-promise`. - recieve-native-query + receive-native-query (fn [qp] (fn [query] (let [results-promise (promise) @@ -183,7 +183,7 @@ ;; everyone a favor and throw an Exception (let [results (m/dissoc-in results [:query :results-promise])] (throw (ex-info (str (tru "Error preprocessing query")) results)))))))] - (recieve-native-query (qp-pipeline deliver-native-query)))) + (receive-native-query (qp-pipeline deliver-native-query)))) (defn query->preprocessed "Return the fully preprocessed form for `query`, the way it would look immediately before `mbql->native` is called. diff --git a/src/metabase/query_processor/middleware/cumulative_aggregations.clj b/src/metabase/query_processor/middleware/cumulative_aggregations.clj index 923325f3c37a599734347f11c0a1bbfd4ed0be67..f4eb6a94cc0ccffb23ebb200ac9516f949b5d7da 100644 --- a/src/metabase/query_processor/middleware/cumulative_aggregations.clj +++ b/src/metabase/query_processor/middleware/cumulative_aggregations.clj @@ -31,16 +31,14 @@ [[index & more] last-row row] (if-not index row - (recur more last-row (update row index (partial + (nth last-row index)))))) + (recur more last-row (update (vec row) index (partial + (nth last-row index)))))) (defn- sum-rows "Sum the values in `rows` at `indexes-to-sum`. (sum-rows #{0} [[1] [2] [3]]) ; -> [[1] [3] [6]]" [indexes-to-sum rows] - (reductions (partial add-rows indexes-to-sum) - (first rows) - (rest rows))) + (reductions (partial add-rows indexes-to-sum) rows)) (defn handle-cumulative-aggregations "Middleware that implements `cum-count` and `cum-sum` aggregations. These clauses are replaced with `count` and `sum` diff --git a/src/metabase/query_processor/middleware/parameters/sql.clj b/src/metabase/query_processor/middleware/parameters/sql.clj index 3763d0b6b07da3c07e74976803854c65fc0fc612..f916796c6c576d645de42afd7bdc1de86fdb9d7b 100644 --- a/src/metabase/query_processor/middleware/parameters/sql.clj +++ b/src/metabase/query_processor/middleware/parameters/sql.clj @@ -191,10 +191,10 @@ "Return the `:default` value for a param if no explicit values were passsed. This only applies to non-Dimension (non-Field Filter) params. Default values for Dimension (Field Filter) params are handled above in `default-value-for-dimension`." - [{:keys [default display_name required]} :- TagParam] + [{:keys [default display-name required]} :- TagParam] (or default (when required - (throw (Exception. (str (tru "''{0}'' is a required param." display_name))))))) + (throw (Exception. (str (tru "''{0}'' is a required param." display-name))))))) ;;; Parsing Values diff --git a/src/metabase/query_processor/util.clj b/src/metabase/query_processor/util.clj index 283b0f3a08a0642d99e2b9f1db38271ac22610d5..c47482e887ac59b3a97eee8314f6359685d4eb70 100644 --- a/src/metabase/query_processor/util.clj +++ b/src/metabase/query_processor/util.clj @@ -56,19 +56,6 @@ (recur (assoc m :query source-query) ks not-found) (get-in m (cons :query ks) not-found)))) -(defn assoc-in-query - "Similar to `assoc-in but will look in either `:query` or recursively in `[:query :source-query]`. Using - this function will avoid having to check if there's a nested query vs. top-level query." - [m ks v] - (if-let [source-query (get-in m [:query :source-query])] - ;; We have a soure-query, we need to recursively `assoc-in` with the source query as the query - (assoc-in m - [:query :source-query] - (-> (assoc m :query source-query) - (assoc-in-query ks v) - :query)) - (assoc-in m (cons :query ks) v))) - ;;; ---------------------------------------------------- Hashing ----------------------------------------------------- diff --git a/test/metabase/driver/bigquery_test.clj b/test/metabase/driver/bigquery_test.clj index 1b711da80f5c73a7bed974cebe41821c34790491..f0c22edefaf509049559981c79eec1273271c54b 100644 --- a/test/metabase/driver/bigquery_test.clj +++ b/test/metabase/driver/bigquery_test.clj @@ -151,13 +151,13 @@ ;; alias, e.g. something like `categories__via__category_id`, which is considerably different from what other SQL ;; databases do. (#4218) (expect-with-engine :bigquery - (str "SELECT `test_data.categories__via__category_id`.`name` AS `categories___name`," + (str "SELECT `categories__via__category_id`.`name` AS `name`," " count(*) AS `count` " "FROM `test_data.venues` " - "LEFT JOIN `test_data.categories` `test_data.categories__via__category_id`" - " ON `test_data.venues`.`category_id` = `test_data.categories__via__category_id`.`id` " - "GROUP BY `categories___name` " - "ORDER BY `categories___name` ASC") + "LEFT JOIN `test_data.categories` `categories__via__category_id`" + " ON `test_data.venues`.`category_id` = `categories__via__category_id`.`id` " + "GROUP BY `name` " + "ORDER BY `name` ASC") ;; normally for test purposes BigQuery doesn't support foreign keys so override the function that checks that and ;; make it return `true` so this test proceeds as expected (with-redefs [driver/driver-supports? (constantly true) @@ -216,3 +216,38 @@ :details (assoc (:details (Database (data/id))) :use-jvm-timezone true)}]] (native-timestamp-query db "2018-08-31 00:00:00+07" "Asia/Jakarta")))) + +;; if I run a BigQuery query, does it get a remark added to it? +(defn- query->native [query] + (with-local-vars [native-query nil] + (with-redefs [bigquery/process-native* (fn [_ sql] + (var-set native-query sql) + (throw (Exception. "Done.")))] + (qp/process-query {:database (data/id) + :type :query + :query {:source-table (data/id :venues) + :limit 1} + :info {:executed-by 1000 + :query-type "MBQL" + :query-hash (byte-array [1 2 3 4])}}) + @native-query))) + +(expect-with-engine :bigquery + (str + "-- Metabase:: userID: 1000 queryType: MBQL queryHash: 01020304\n" + "SELECT `test_data.venues`.`id` AS `id`," + " `test_data.venues`.`name` AS `name`," + " `test_data.venues`.`category_id` AS `category_id`," + " `test_data.venues`.`latitude` AS `latitude`," + " `test_data.venues`.`longitude` AS `longitude`," + " `test_data.venues`.`price` AS `price` " + "FROM `test_data.venues` " + "LIMIT 1") + (query->native + {:database (data/id) + :type :query + :query {:source-table (data/id :venues) + :limit 1} + :info {:executed-by 1000 + :query-type "MBQL" + :query-hash (byte-array [1 2 3 4])}})) diff --git a/test/metabase/models/query/permissions_test.clj b/test/metabase/models/query/permissions_test.clj index ad67536744e4eef20ddbef8a9f2a3a8c1237c0c3..c95efc541db2c871d4a9cb1d83f8b8855d41c171 100644 --- a/test/metabase/models/query/permissions_test.clj +++ b/test/metabase/models/query/permissions_test.clj @@ -13,9 +13,9 @@ [metabase.models.query.permissions :as query-perms] [metabase.test.data :as data] [metabase.test.data.users :as users] + [metabase.test.util.log :as tu.log] [metabase.util :as u] - [toucan.util.test :as tt] - [metabase.test.util.log :as tu.log])) + [toucan.util.test :as tt])) ;;; ---------------------------------------------- Permissions Checking ---------------------------------------------- diff --git a/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj b/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj index 72a3e144a4205e54b4cef9465447006ca7063ad0..ae0921af28c8257043adfe9fcb71552eee280be1 100644 --- a/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj +++ b/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj @@ -18,6 +18,7 @@ [1 4 6] (#'cumulative-aggregations/add-rows #{1 2} [1 2 3] [1 2 3])) +;; should throw an Exception if index is out of bounds (expect IndexOutOfBoundsException (#'cumulative-aggregations/add-rows #{4} [1 2 3] [1 2 3])) @@ -50,6 +51,34 @@ #{0 1} [[0 0] [1 1] [2 2] [3 3] [4 4] [5 5] [6 6] [7 7] [8 8] [9 9]])) +;; make sure cumulative aggregations still work correctly with lists... +(expect + [[1 1 1] [2 3 2] [3 6 3]] + (#'cumulative-aggregations/sum-rows #{1} '((1 1 1) (2 2 2) (3 3 3)))) + +;; ...and lazy sequences +(expect + [[1 1 1] [2 3 2] [3 6 3]] + (#'cumulative-aggregations/sum-rows #{1} (lazy-cat '((1 1 1)) '((2 2 2)) '((3 3 3))))) + +;; the results should be L A Z Y +(expect + {:fully-realized-after-taking-2? false + :fully-realized-after-taking-3? true} + (with-local-vars [fully-realized? false] + (let [a-lazy-seq (lazy-cat + '((1 1 1)) + '((2 2 2)) + (do + (var-set fully-realized? true) + '((3 3 3)))) + realize-n (fn [n] + (dorun (take n (#'cumulative-aggregations/sum-rows #{1} a-lazy-seq))) + @fully-realized?)] + {:fully-realized-after-taking-2? (realize-n 2) + :fully-realized-after-taking-3? (realize-n 3)}))) + + ;; can it go forever without a stack overflow? (expect [[4999850001] [4999950000]] diff --git a/test/metabase/query_processor/util_test.clj b/test/metabase/query_processor/util_test.clj index 68d5fa188ae05f1d611eeda3e626a0a74a168c8b..2c3c18c6f53fc568ad626d57eb20b6bc853776e0 100644 --- a/test/metabase/query_processor/util_test.clj +++ b/test/metabase/query_processor/util_test.clj @@ -147,21 +147,3 @@ (expect not-found (qputil/get-in-query {} [:test] not-found))) - -(def ^:private updated-test-map - {:test {:value 11}}) - -;; assoc-in-query works with a non-nested query -(expect - {:query updated-test-map} - (qputil/assoc-in-query {:query test-inner-map} [:test :value] 11)) - -;; assoc-in-query works with a nested query -(expect - {:query {:source-query updated-test-map}} - (qputil/assoc-in-query {:query {:source-query test-inner-map}} [:test :value] 11)) - -;; Not supported yet, but assoc-in-query should do the right thing with a double nested query -(expect - {:query {:source-query {:source-query updated-test-map}}} - (qputil/assoc-in-query {:query {:source-query {:source-query test-inner-map}}} [:test :value] 11)) diff --git a/test/metabase/query_processor_test.clj b/test/metabase/query_processor_test.clj index 469eb3ddd34ebceed2f402c9c4733371f0a8ff79..778f86b3e16ff7e4bc58f0fd03c6468f098c0aa2 100644 --- a/test/metabase/query_processor_test.clj +++ b/test/metabase/query_processor_test.clj @@ -9,7 +9,9 @@ [driver :as driver] [util :as u]] [metabase.test.data :as data] - [metabase.test.data.datasets :as datasets] + [metabase.test.data + [datasets :as datasets] + [interface :as di]] [metabase.util.date :as du])) ;; make sure all the driver test extension namespaces are loaded <3 if this isn't done some things will get loaded at @@ -272,27 +274,9 @@ (aggregate-col :count) (aggregate-col :avg (venues-col :id))" - {:arglists '([ag-col-kw] [ag-col-kw field])} - ;; TODO - cumulative count doesn't require a FIELD !!!!!!!!! - ([ag-col-kw] - (assert (= ag-col-kw) :count) - {:base_type :type/Integer - :special_type :type/Number - :name "count" - :display_name "count" - :source :aggregation}) - ([ag-col-kw {:keys [base_type special_type]}] - {:pre [base_type special_type]} - (merge - {:base_type base_type - :special_type special_type - :settings nil - :name (name ag-col-kw) - :display_name (name ag-col-kw) - :source :aggregation} - ;; count always gets the same special type regardless - (when (= ag-col-kw :count) - (aggregate-col :count))))) + {:arglists '([ag-type] [ag-type field])} + [& args] + (apply di/aggregate-column-info datasets/*driver* args)) (defn breakout-col [col] (assoc col :source :breakout)) diff --git a/test/metabase/query_processor_test/nested_queries_test.clj b/test/metabase/query_processor_test/nested_queries_test.clj index a7f6705523c6cbc6b1cc8619c86c37a4c1307cae..c657c0e6a1dcfe643642d69e594ecb9ef8d7c2f9 100644 --- a/test/metabase/query_processor_test/nested_queries_test.clj +++ b/test/metabase/query_processor_test/nested_queries_test.clj @@ -646,3 +646,22 @@ Collection [dest-card-collection]] (perms/grant-collection-read-permissions! (group/all-users) source-card-collection) (save-card-via-API-with-native-source-query! 403 db source-card-collection dest-card-collection))))) + +;; make sure that if we refer to a Field that is actually inside the source query, the QP is smart enough to figure +;; out what you were referring to and behave appropriately +(datasets/expect-with-engines (non-timeseries-engines-with-feature :nested-queries) + [[10]] + (format-rows-by [int] + (rows + (qp/process-query + {:database (data/id) + :type :query + :query {:source-query {:source-table (data/id :venues) + :fields [(data/id :venues :id) + (data/id :venues :name) + (data/id :venues :category_id) + (data/id :venues :latitude) + (data/id :venues :longitude) + (data/id :venues :price)]} + :aggregation [[:count]] + :filter [:= [:field-id (data/id :venues :category_id)] 50]}})))) diff --git a/test/metabase/query_processor_test/order_by_test.clj b/test/metabase/query_processor_test/order_by_test.clj index d3edcadb6a15eca87cb98704d83991e83a947bf4..2ef8d3f7c2c22a3bc35161ce3a9dc44551192e6d 100644 --- a/test/metabase/query_processor_test/order_by_test.clj +++ b/test/metabase/query_processor_test/order_by_test.clj @@ -115,10 +115,10 @@ (datasets/expect-with-engines (non-timeseries-engines-with-feature :standard-deviation-aggregations) {:columns [(data/format-name "price") "stddev"] - :rows [[3 (if (contains? #{:mysql :crate} *engine*) 25 26)] + :rows [[3 (if (#{:mysql :crate} *engine*) 25 26)] [1 24] [2 21] - [4 (if (contains? #{:mysql :crate} *engine*) 14 15)]] + [4 (if (#{:mysql :crate} *engine*) 14 15)]] :cols [(breakout-col (venues-col :price)) (aggregate-col :stddev (venues-col :category_id))] :native_form true} @@ -129,4 +129,4 @@ booleanize-native-form data (format-rows-by [int (comp int math/round)]) - tu/round-fingerprint-cols)) \ No newline at end of file + tu/round-fingerprint-cols)) diff --git a/test/metabase/test/data/bigquery.clj b/test/metabase/test/data/bigquery.clj index 23e0e38d253b1e68b8b1071a8f9c95a4f3385902..9f9fb8a2ea7116f8731c336eba2ef2fc86e440b9 100644 --- a/test/metabase/test/data/bigquery.clj +++ b/test/metabase/test/data/bigquery.clj @@ -10,6 +10,7 @@ [metabase.test.data [datasets :as datasets] [interface :as i]] + [metabase.query-processor-test :as qp.test] [metabase.util :as u] [metabase.util [date :as du] @@ -233,6 +234,17 @@ (destroy-dataset! database-name)) (throw e))))))) +(defn aggregate-column-info + ([driver aggregation-type] + (i/default-aggregate-column-info driver aggregation-type)) + ([driver aggregation-type field] + (merge + (i/default-aggregate-column-info driver aggregation-type field) + ;; BigQuery averages, standard deviations come back as Floats. This might apply to some other ag types as well; + ;; add them as we come across them. + (when (#{:avg :stddev} aggregation-type) + {:base_type :type/Float})))) + ;;; --------------------------------------------- IDriverTestExtensions ---------------------------------------------- @@ -241,4 +253,5 @@ (merge i/IDriverTestExtensionsDefaultsMixin {:engine (constantly :bigquery) :database->connection-details (u/drop-first-arg database->connection-details) - :create-db! (u/drop-first-arg create-db!)})) + :create-db! (u/drop-first-arg create-db!) + :aggregate-column-info aggregate-column-info})) diff --git a/test/metabase/test/data/dataset_definitions.clj b/test/metabase/test/data/dataset_definitions.clj index 7da35d959bf27adcc4f7732f29455455b37dce78..0d4c6b2f719ce680edae59f0295610e178d628a5 100644 --- a/test/metabase/test/data/dataset_definitions.clj +++ b/test/metabase/test/data/dataset_definitions.clj @@ -1,8 +1,6 @@ (ns metabase.test.data.dataset-definitions "Definitions of various datasets for use in tests with `with-temp-db`." - (:require [clojure.tools.reader.edn :as edn] - [metabase.test.data.interface :as di] - [metabase.util.date :as du]) + (:require [metabase.test.data.interface :as di]) (:import java.sql.Time java.util.Calendar)) @@ -23,7 +21,7 @@ (di/def-database-definition-edn places-cam-likes) ;; A small dataset with users and a set of messages between them. Each message has *2* foreign keys to user -- -;; sender and reciever -- allowing us to test situations where multiple joins for a *single* table should occur. +;; sender and receiver -- allowing us to test situations where multiple joins for a *single* table should occur. (di/def-database-definition-edn avian-singles) (defn- date-only diff --git a/test/metabase/test/data/interface.clj b/test/metabase/test/data/interface.clj index 7d538d13269fcaeb4ad7324355fc920dbad84453..d8a3465dd79869c508794ce0b28fd6d4ef40ab42 100644 --- a/test/metabase/test/data/interface.clj +++ b/test/metabase/test/data/interface.clj @@ -71,6 +71,30 @@ ([session-schema _ db-name table-name] [session-schema (db-qualified-table-name db-name table-name)]) ([session-schema _ db-name table-name field-name] [session-schema (db-qualified-table-name db-name table-name) field-name])) +(defn default-aggregate-column-info + "Default implementation of `aggregate-column-info` for drivers using the `IDriverTestExtensionsDefaultsMixin`." + {:arglists '([driver aggregation-type] [driver aggregation-type field])} + ([_ aggregation-type] + ;; TODO - cumulative count doesn't require a FIELD !!!!!!!!! + (assert (= aggregation-type) :count) + {:base_type :type/Integer + :special_type :type/Number + :name "count" + :display_name "count" + :source :aggregation}) + ([driver aggregation-type {:keys [base_type special_type]}] + {:pre [base_type special_type]} + (merge + {:base_type base_type + :special_type special_type + :settings nil + :name (name aggregation-type) + :display_name (name aggregation-type) + :source :aggregation} + ;; count always gets the same special type regardless + (when (= aggregation-type :count) + (default-aggregate-column-info driver :count))))) + (defprotocol IMetabaseInstance (metabase-instance [this context] @@ -148,7 +172,11 @@ (id-field-type ^clojure.lang.Keyword [this] "*OPTIONAL* Return the `base_type` of the `id` `Field` (e.g. `:type/Integer` or `:type/BigInteger`). Defaults to - `:type/Integer`.")) + `:type/Integer`.") + + (aggregate-column-info [this aggregation-type] [this aggregation-type field] + "*OPTIONAL*. Return the expected type information that should come back for QP results as part of `:cols` for an + aggregation of a given type (and applied to a given Field, when applicable).")) (def IDriverTestExtensionsDefaultsMixin "Default implementations for the `IDriverTestExtensions` methods marked *OPTIONAL*." @@ -157,7 +185,8 @@ :format-name (u/drop-first-arg identity) :has-questionable-timezone-support? (fn [driver] (not (contains? (driver/features driver) :set-timezone))) - :id-field-type (constantly :type/Integer)}) + :id-field-type (constantly :type/Integer) + :aggregate-column-info default-aggregate-column-info}) ;; ## Helper Functions for Creating New Definitions