From 69fc2b2a03835bd887d5f2a424b3c3f00841f31e Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 2 May 2024 17:50:45 -0700
Subject: [PATCH] Window functions should use all breakouts except first for
partitioning (#42131)
* Window functions should use all breakouts except first for partitioning
* Update the offset() test
* Update tests
* Rework how we use driver escape-alias in MLv2; fix Oracle
* PR feedback
* Test fixes :wrench:
---
src/metabase/driver/sql/query_processor.clj | 11 +-
src/metabase/lib/convert.cljc | 2 +-
src/metabase/lib/database/methods.cljc | 19 +++
src/metabase/lib/extraction.cljc | 10 +-
src/metabase/lib/field.cljc | 6 +-
src/metabase/lib/join.cljc | 23 +--
src/metabase/lib/join/util.cljc | 6 +-
src/metabase/lib/js.cljs | 2 +-
src/metabase/lib/metadata/calculation.cljc | 26 ++--
.../lib/metadata/invocation_tracker.cljc | 11 +-
src/metabase/lib/metadata/jvm.clj | 5 +-
src/metabase/lib/remove_replace.cljc | 7 +-
src/metabase/lib/schema/metadata.cljc | 19 ++-
src/metabase/lib/stage.cljc | 16 +--
src/metabase/lib/util.cljc | 81 ++++++++---
.../middleware/add_dimension_projections.clj | 6 +-
.../query_processor/middleware/annotate.clj | 3 +-
.../middleware/cumulative_aggregations.clj | 73 ++++++----
.../middleware/escape_join_aliases.clj | 11 +-
src/metabase/query_processor/setup.clj | 10 --
.../query_processor/util/add_alias_info.clj | 18 +--
.../util/transformations/nest_breakouts.clj | 1 -
test/metabase/lib/util_test.cljc | 26 +++-
.../middleware/annotate_test.clj | 25 ++--
.../cumulative_aggregations_test.clj | 97 ++++++++-----
.../middleware/escape_join_aliases_test.clj | 44 ++++--
.../transformations/nest_breakouts_test.clj | 84 +++++++++++
.../cumulative_aggregation_test.clj | 136 ++++++++++--------
.../query_processor_test/offset_test.clj | 30 ++--
29 files changed, 536 insertions(+), 272 deletions(-)
create mode 100644 src/metabase/lib/database/methods.cljc
diff --git a/src/metabase/driver/sql/query_processor.clj b/src/metabase/driver/sql/query_processor.clj
index 23786f8db66..d156247935c 100644
--- a/src/metabase/driver/sql/query_processor.clj
+++ b/src/metabase/driver/sql/query_processor.clj
@@ -690,12 +690,15 @@
(->honeysql driver mbql-expr)
(->honeysql driver power)])
-(defn- window-aggregation-over-expr-for-query-with-breakouts [driver inner-query]
+(defn- window-aggregation-over-expr-for-query-with-breakouts
+ "Order by the first breakout, then partition by all the other ones. See #42003 and
+ https://metaboat.slack.com/archives/C05MPF0TM3L/p1714084449574689 for more info."
+ [driver inner-query]
(let [num-breakouts (count (:breakout inner-query))
group-bys (:group-by (apply-top-level-clause driver :breakout {} inner-query))
- partition-exprs (when ((fnil > 0) num-breakouts 1)
- (butlast group-bys))
- order-expr (last group-bys)]
+ partition-exprs (when (> num-breakouts 1)
+ (rest group-bys))
+ order-expr (first group-bys)]
(merge
(when (seq partition-exprs)
{:partition-by (mapv (fn [expr]
diff --git a/src/metabase/lib/convert.cljc b/src/metabase/lib/convert.cljc
index c357b1fe08b..407ca2abc8e 100644
--- a/src/metabase/lib/convert.cljc
+++ b/src/metabase/lib/convert.cljc
@@ -139,7 +139,7 @@
Only deduplicate the default `__join` aliases; we don't want the [[lib.util/unique-name-generator]] to touch other
aliases and truncate them or anything like that."
[joins]
- (let [unique-name-fn (lib.util/unique-name-generator)]
+ (let [unique-name-fn (lib.util/unique-name-generator nil)]
(mapv (fn [join]
(cond-> join
(= (:alias join) legacy-default-join-alias) (update :alias unique-name-fn)))
diff --git a/src/metabase/lib/database/methods.cljc b/src/metabase/lib/database/methods.cljc
new file mode 100644
index 00000000000..6ff11769271
--- /dev/null
+++ b/src/metabase/lib/database/methods.cljc
@@ -0,0 +1,19 @@
+(ns metabase.lib.database.methods
+ "Wrappers around [[metabase.driver]] methods that we may need to use inside MLv2 such
+ as [[metabase.driver/escape-alias]], so we can decouple the driver interface from MLv2. Since driver methods are
+ Clojure-only, we should only expect these to be bound in Clojure-land usage (e.g. the QP) and not in Cljs usage.
+ MetadataProviders can pass these methods in as part of the database under the `:lib/methods` key. See the
+ `:metabase.lib.schema.metadata/database.methods` schema for more info."
+ (:require
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
+ [metabase.util.malli :as mu]))
+
+(mu/defn escape-alias :- :string
+ "MLv2-friendly wrapper around [[metabase.driver/escape-alias]]. By default this is `identity` but metadata providers
+ can override this by including `[:lib/methods :escape-alias]` as part of the Database metadata.
+
+ This is used for escaping a unique alias when generating `:lib/desired-alias`."
+ ^String [database :- [:maybe ::lib.schema.metadata/database]
+ s :- :string]
+ (let [f (get-in database [:lib/methods :escape-alias] identity)]
+ (f s)))
diff --git a/src/metabase/lib/extraction.cljc b/src/metabase/lib/extraction.cljc
index 275bb25aaf8..eba1221308c 100644
--- a/src/metabase/lib/extraction.cljc
+++ b/src/metabase/lib/extraction.cljc
@@ -105,9 +105,9 @@
(let [unique-name-fn (->> (lib.util/query-stage query stage-number)
(lib.metadata.calculation/returned-columns query stage-number)
(map :name)
- lib.util/unique-name-generator)]
+ (lib.util/unique-name-generator (lib.metadata/->metadata-provider query)))]
(lib.expression/expression
- query
- stage-number
- (unique-name-fn display-name)
- (extraction-expression extraction))))
+ query
+ stage-number
+ (unique-name-fn display-name)
+ (extraction-expression extraction))))
diff --git a/src/metabase/lib/field.cljc b/src/metabase/lib/field.cljc
index d4e01edaa2c..5d6d6eb46c8 100644
--- a/src/metabase/lib/field.cljc
+++ b/src/metabase/lib/field.cljc
@@ -458,7 +458,8 @@
(:lib/source metadata))
(:source-alias metadata))]
{:join-alias source-alias})
- (when-let [join-alias (lib.join.util/current-join-alias metadata)]
+ (when-let [join-alias (when-not inherited-column?
+ (lib.join.util/current-join-alias metadata))]
{:join-alias join-alias})
(when-let [temporal-unit (::temporal-unit metadata)]
{:temporal-unit temporal-unit})
@@ -466,7 +467,8 @@
{::original-effective-type original-effective-type})
(when-let [binning (::binning metadata)]
{:binning binning})
- (when-let [source-field-id (:fk-field-id metadata)]
+ (when-let [source-field-id (when-not inherited-column?
+ (:fk-field-id metadata))]
{:source-field source-field-id}))
id-or-name ((if inherited-column?
(some-fn :lib/desired-column-alias :name)
diff --git a/src/metabase/lib/join.cljc b/src/metabase/lib/join.cljc
index bf7924a0382..cf79de274ad 100644
--- a/src/metabase/lib/join.cljc
+++ b/src/metabase/lib/join.cljc
@@ -230,7 +230,7 @@
[:alias
{:error/message "Join must have an alias to determine column aliases!"}
::lib.schema.common/non-blank-string]]
- unique-name-fn :- fn?
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn
col :- :map]
(assoc col
:lib/source-column-alias ((some-fn :lib/source-column-alias :name) col)
@@ -238,11 +238,12 @@
(:alias join)
((some-fn :lib/source-column-alias :name) col)))))
-(defmethod lib.metadata.calculation/returned-columns-method :mbql/join
+(mu/defmethod lib.metadata.calculation/returned-columns-method :mbql/join
[query
stage-number
{:keys [fields stages], join-alias :alias, :or {fields :none}, :as join}
- {:keys [unique-name-fn], :as options}]
+ {:keys [unique-name-fn], :as options} :- [:map
+ [:unique-name-fn ::lib.metadata.calculation/unique-name-fn]]]
(when-not (= fields :none)
(let [ensure-previous-stages-have-metadata (resolve 'metabase.lib.stage/ensure-previous-stages-have-metadata)
join-query (cond-> (assoc query :stages stages)
@@ -266,7 +267,7 @@
"Convenience for calling [[lib.metadata.calculation/visible-columns]] on all of the joins in a query stage."
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(into []
(mapcat (fn [join]
(lib.metadata.calculation/visible-columns query
@@ -469,8 +470,8 @@
;; we leave alone the condition otherwise
:else &match)))))
-(defn- generate-unique-name [base-name taken-names]
- (let [generator (lib.util/unique-name-generator)]
+(defn- generate-unique-name [metadata-providerable base-name taken-names]
+ (let [generator (lib.util/unique-name-generator (lib.metadata/->metadata-provider metadata-providerable))]
(run! generator taken-names)
(generator base-name)))
@@ -487,8 +488,8 @@
home-cols (lib.metadata.calculation/visible-columns query stage-number stage)
cond-fields (lib.util.match/match (:conditions a-join) :field)
home-col (select-home-column home-cols cond-fields)
- join-alias (-> (calculate-join-alias query a-join home-col)
- (generate-unique-name (keep :alias (:joins stage))))
+ join-alias (as-> (calculate-join-alias query a-join home-col) s
+ (generate-unique-name query s (keep :alias (:joins stage))))
join-cols (lib.metadata.calculation/returned-columns
(lib.query/query-with-stages query (:stages a-join)))]
(-> a-join
@@ -843,10 +844,10 @@
(filter-clause (::target fk) fk))
fks)))))))
-(defn- xform-add-join-alias [a-join]
+(defn- xform-add-join-alias [metadata-providerable a-join]
(let [join-alias (lib.join.util/current-join-alias a-join)]
(fn [xf]
- (let [unique-name-fn (lib.util/unique-name-generator)]
+ (let [unique-name-fn (lib.util/unique-name-generator (lib.metadata/->metadata-provider metadata-providerable))]
(fn
([] (xf))
([result] (xf result))
@@ -885,7 +886,7 @@
(into []
(if a-join
(comp xform-fix-source-for-joinable-columns
- (xform-add-join-alias a-join)
+ (xform-add-join-alias query a-join)
(xform-mark-selected-joinable-columns a-join))
identity)
cols)))
diff --git a/src/metabase/lib/join/util.cljc b/src/metabase/lib/join/util.cljc
index 43293dc4e69..8aeb8a657a6 100644
--- a/src/metabase/lib/join/util.cljc
+++ b/src/metabase/lib/join/util.cljc
@@ -49,7 +49,8 @@
MyJoin__my_field
- You should pass the results thru a unique name function."
+ You should pass the results thru a unique name function e.g. one returned
+ by [[metabase.lib.util/unique-name-generator]]."
[join-alias :- ::lib.schema.common/non-blank-string
field-name :- ::lib.schema.common/non-blank-string]
(lib.util/format "%s__%s" join-alias field-name))
@@ -59,7 +60,8 @@
CATEGORIES__via__CATEGORY_ID
- You should make sure this gets ran thru a unique-name fn."
+ You should make sure this gets ran thru a unique-name fn e.g. one returned
+ by [[metabase.lib.util/unique-name-generator]]."
[table-name :- ::lib.schema.common/non-blank-string
source-field-id-name :- ::lib.schema.common/non-blank-string]
(lib.util/format "%s__via__%s" table-name source-field-id-name))
diff --git a/src/metabase/lib/js.cljs b/src/metabase/lib/js.cljs
index 0487dd5591b..244702a4d9e 100644
--- a/src/metabase/lib/js.cljs
+++ b/src/metabase/lib/js.cljs
@@ -1085,7 +1085,7 @@
"Inner implementation for [[returned-columns]], which wraps this with caching."
[a-query stage-number]
(let [stage (lib.util/query-stage a-query stage-number)
- unique-name-fn (lib.util/unique-name-generator)]
+ unique-name-fn (lib.util/unique-name-generator (lib.metadata/->metadata-provider a-query))]
(->> (lib.metadata.calculation/returned-columns a-query stage-number stage)
(map #(-> %
(assoc :selected? true)
diff --git a/src/metabase/lib/metadata/calculation.cljc b/src/metabase/lib/metadata/calculation.cljc
index c3ea6e8f872..b0cbb2e8242 100644
--- a/src/metabase/lib/metadata/calculation.cljc
+++ b/src/metabase/lib/metadata/calculation.cljc
@@ -12,8 +12,7 @@
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.expression :as lib.schema.expresssion]
[metabase.lib.schema.metadata :as lib.schema.metadata]
- [metabase.lib.schema.temporal-bucketing
- :as lib.schema.temporal-bucketing]
+ [metabase.lib.schema.temporal-bucketing :as lib.schema.temporal-bucketing]
[metabase.lib.types.isa :as lib.types.isa]
[metabase.lib.util :as lib.util]
[metabase.shared.util.i18n :as i18n]
@@ -427,20 +426,25 @@
(empty? columns)
(apply distinct? (map (comp u/lower-case-en :lib/desired-column-alias) columns))))]])
-(def ^:private UniqueNameFn
+(mr/def ::unique-name-fn
+ "Stateful function with the signature
+
+ (f str) => unique-str
+
+ i.e. repeated calls with the same string should return different unique strings."
[:=>
- [:cat ::lib.schema.common/non-blank-string]
+ [:cat :string] ; this is allowed to be a blank string.
::lib.schema.common/non-blank-string])
(def ReturnedColumnsOptions
"Schema for options passed to [[returned-columns]] and [[returned-columns-method]]."
[:map
;; has the signature (f str) => str
- [:unique-name-fn {:optional true} UniqueNameFn]])
+ [:unique-name-fn {:optional true} ::unique-name-fn]])
(mu/defn ^:private default-returned-columns-options :- ReturnedColumnsOptions
- []
- {:unique-name-fn (lib.util/unique-name-generator)})
+ [metadata-providerable]
+ {:unique-name-fn (lib.util/unique-name-generator (lib.metadata/->metadata-provider metadata-providerable))})
(defmulti returned-columns-method
"Impl for [[returned-columns]]."
@@ -477,7 +481,7 @@
stage-number :- :int
x
options :- [:maybe ReturnedColumnsOptions]]
- (let [options (merge (default-returned-columns-options) options)]
+ (let [options (merge (default-returned-columns-options query) options)]
(returned-columns-method query stage-number x options))))
(def VisibleColumnsOptions
@@ -492,9 +496,9 @@
[:include-implicitly-joinable-for-source-card? {:optional true} :boolean]]])
(mu/defn ^:private default-visible-columns-options :- VisibleColumnsOptions
- []
+ [metadata-providerable]
(merge
- (default-returned-columns-options)
+ (default-returned-columns-options metadata-providerable)
{:include-joined? true
:include-expressions? true
:include-implicitly-joinable? true
@@ -561,7 +565,7 @@
stage-number :- :int
x
options :- [:maybe VisibleColumnsOptions]]
- (let [options (merge (default-visible-columns-options) options)]
+ (let [options (merge (default-visible-columns-options query) options)]
(visible-columns-method query stage-number x options))))
(mu/defn primary-keys :- [:sequential ::lib.schema.metadata/column]
diff --git a/src/metabase/lib/metadata/invocation_tracker.cljc b/src/metabase/lib/metadata/invocation_tracker.cljc
index c76f77a7550..0fdd5d081b3 100644
--- a/src/metabase/lib/metadata/invocation_tracker.cljc
+++ b/src/metabase/lib/metadata/invocation_tracker.cljc
@@ -1,5 +1,6 @@
(ns metabase.lib.metadata.invocation-tracker
(:require
+ #?(:clj [pretty.core :as pretty])
[metabase.lib.metadata.protocols :as lib.metadata.protocols]))
(def ^:private ^:dynamic *to-track-metadata-types*
@@ -41,7 +42,7 @@
id))
(deftype InvocationTracker [tracker metadata-provider]
- lib.metadata.protocols/InvocationTracker
+ lib.metadata.protocols/InvocationTracker
(invoked-ids [_this metadata-type]
(get @tracker metadata-type))
@@ -64,14 +65,18 @@
(store-database! [_this database-metadata] (lib.metadata.protocols/store-database! metadata-provider database-metadata))
(store-metadata! [_this metadata-type id metadata] (lib.metadata.protocols/store-metadata! metadata-provider metadata-type id metadata))
-
lib.metadata.protocols/BulkMetadataProvider
(bulk-metadata [_this metadata-type ids] (lib.metadata.protocols/bulk-metadata metadata-provider metadata-type ids))
#?(:clj Object :cljs IEquiv)
(#?(:clj equals :cljs -equiv) [_this another]
(and (instance? InvocationTracker another)
- (= metadata-provider (.metadata-provider ^InvocationTracker another)))))
+ (= metadata-provider (.metadata-provider ^InvocationTracker another))))
+
+ #?@(:clj
+ [pretty/PrettyPrintable
+ (pretty [_this]
+ (list `invocation-tracker-provider metadata-provider))]))
(defn invocation-tracker-provider
"Wraps `metadata-provider` with a provider that records all invoked ids of [[lib.metadata.protocols/MetadataProvider]] methods."
diff --git a/src/metabase/lib/metadata/jvm.clj b/src/metabase/lib/metadata/jvm.clj
index cf1f3e1228c..f679e966fa7 100644
--- a/src/metabase/lib/metadata/jvm.clj
+++ b/src/metabase/lib/metadata/jvm.clj
@@ -2,6 +2,8 @@
"Implementation(s) of [[metabase.lib.metadata.protocols/MetadataProvider]] only for the JVM."
(:require
[clojure.string :as str]
+ #_{:clj-kondo/ignore [:discouraged-namespace]}
+ [metabase.driver :as driver]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.cached-provider :as lib.metadata.cached-provider]
[metabase.lib.metadata.invocation-tracker :as lib.metadata.invocation-tracker]
@@ -72,7 +74,8 @@
[database]
;; ignore encrypted details that we cannot decrypt, because that breaks schema
;; validation
- (let [database (instance->metadata database :metadata/database)]
+ (let [database (instance->metadata database :metadata/database)
+ database (assoc database :lib/methods {:escape-alias (partial driver/escape-alias (:engine database))})]
(cond-> database
(not (map? (:details database))) (dissoc :details))))
diff --git a/src/metabase/lib/remove_replace.cljc b/src/metabase/lib/remove_replace.cljc
index eb1fdb0844c..306a0dd6635 100644
--- a/src/metabase/lib/remove_replace.cljc
+++ b/src/metabase/lib/remove_replace.cljc
@@ -9,6 +9,7 @@
[metabase.lib.expression :as lib.expression]
[metabase.lib.join :as lib.join]
[metabase.lib.join.util :as lib.join.util]
+ [metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.options :as lib.options]
[metabase.lib.ref :as lib.ref]
@@ -398,12 +399,12 @@
(lib.join/with-join-alias field new-name)))
(defn- rename-join-in-stage
- [stage idx new-name]
+ [metadata-providerable stage idx new-name]
(let [the-joins (:joins stage)
[idx old-name] (when (< -1 idx (count the-joins))
[idx (get-in the-joins [idx :alias])])]
(if (and idx (not= old-name new-name))
- (let [unique-name-fn (lib.util/unique-name-generator)
+ (let [unique-name-fn (lib.util/unique-name-generator (lib.metadata/->metadata-provider metadata-providerable))
_ (run! unique-name-fn (map :alias the-joins))
unique-name (unique-name-fn new-name)]
(-> stage
@@ -438,7 +439,7 @@
join-spec :- [:or :metabase.lib.schema.join/join :string :int]
new-name :- :metabase.lib.schema.common/non-blank-string]
(if-let [idx (join-spec->clause query stage-number join-spec)]
- (lib.util/update-query-stage query stage-number rename-join-in-stage idx new-name)
+ (lib.util/update-query-stage query stage-number (partial rename-join-in-stage query) idx new-name)
query)))
(defn- remove-matching-missing-columns
diff --git a/src/metabase/lib/schema/metadata.cljc b/src/metabase/lib/schema/metadata.cljc
index af76d121a54..09f2503cebe 100644
--- a/src/metabase/lib/schema/metadata.cljc
+++ b/src/metabase/lib/schema/metadata.cljc
@@ -281,6 +281,22 @@
[:display-name {:optional true} [:maybe ::lib.schema.common/non-blank-string]]
[:schema {:optional true} [:maybe ::lib.schema.common/non-blank-string]]])
+(mr/def ::database.methods.escape-alias
+ "MLv2 wrapper around [[metabase.driver/escape-alias]]. Note that this doesn't take `driver` as an argument. It has the
+ signature
+
+ (escape-alias string) => string"
+ [:=> [:cat :string] :string])
+
+(mr/def ::database.methods
+ "A map of wrappers around [[metabase.driver]] methods that we may need to use inside MLv2 such
+ as [[metabase.driver/escape-alias]], so we can decouple the driver interface from MLv2. Since driver methods are
+ Clojure-only, we should only expect these to be bound in Clojure-land usage (e.g. the QP) and not in Cljs usage.
+ MetadataProviders can pass these methods in as part of the database under the `:lib/methods` key. See the
+ `:metabase.lib.schema.metadata/database.methods` schema for more info."
+ [:map
+ [:escape-alias {:optional true} [:ref ::database.methods.escape-alias]]])
+
(mr/def ::database
[:map
{:error/message "Valid Database metadata"}
@@ -293,7 +309,8 @@
[:engine {:optional true} :keyword]
[:features {:optional true} [:set :keyword]]
[:is-audit {:optional true} :boolean]
- [:settings {:optional true} [:maybe :map]]])
+ [:settings {:optional true} [:maybe :map]]
+ [:lib/methods {:optional true} [:maybe [:ref ::database.methods]]]])
(mr/def ::metadata-provider
[:fn
diff --git a/src/metabase/lib/stage.cljc b/src/metabase/lib/stage.cljc
index fb67eb2e41c..91cbfdda587 100644
--- a/src/metabase/lib/stage.cljc
+++ b/src/metabase/lib/stage.cljc
@@ -70,7 +70,7 @@
(mu/defn ^:private breakouts-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(not-empty
(for [breakout (lib.breakout/breakouts-metadata query stage-number)]
(assoc breakout
@@ -81,7 +81,7 @@
(mu/defn ^:private aggregations-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(not-empty
(for [ag (lib.aggregation/aggregations-metadata query stage-number)]
(assoc ag
@@ -94,7 +94,7 @@
(mu/defn ^:private fields-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(when-let [{fields :fields} (lib.util/query-stage query stage-number)]
(not-empty
(for [[tag :as ref-clause] fields
@@ -113,7 +113,7 @@
(mu/defn ^:private summary-columns :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(not-empty
(into []
(mapcat (fn [f]
@@ -125,7 +125,7 @@
"Metadata for the previous stage, if there is one."
[query :- ::lib.schema/query
stage-number :- :int
- unique-name-fn :- fn?]
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(when-let [previous-stage-number (lib.util/previous-stage-number query stage-number)]
(not-empty
(for [col (lib.metadata.calculation/returned-columns query
@@ -161,9 +161,9 @@
(not-empty (lib.metadata.calculation/visible-columns query stage-number card options)))))
(mu/defn ^:private expressions-metadata :- [:maybe lib.metadata.calculation/ColumnsWithUniqueAliases]
- [query :- ::lib.schema/query
- stage-number :- :int
- unique-name-fn :- fn?]
+ [query :- ::lib.schema/query
+ stage-number :- :int
+ unique-name-fn :- ::lib.metadata.calculation/unique-name-fn]
(not-empty
(for [expression (lib.expression/expressions-metadata query stage-number)]
(let [base-type (:base-type expression)]
diff --git a/src/metabase/lib/util.cljc b/src/metabase/lib/util.cljc
index 783b1e90b0f..0ccdf82b5a3 100644
--- a/src/metabase/lib/util.cljc
+++ b/src/metabase/lib/util.cljc
@@ -12,13 +12,16 @@
[medley.core :as m]
[metabase.legacy-mbql.util :as mbql.u]
[metabase.lib.common :as lib.common]
+ [metabase.lib.database.methods :as lib.database.methods]
[metabase.lib.dispatch :as lib.dispatch]
[metabase.lib.hierarchy :as lib.hierarchy]
+ [metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.lib.options :as lib.options]
[metabase.lib.schema :as lib.schema]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.expression :as lib.schema.expression]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
[metabase.lib.schema.ref :as lib.schema.ref]
[metabase.lib.util.match :as lib.util.match]
[metabase.shared.util.i18n :as i18n]
@@ -476,40 +479,72 @@
[query :- :map]
(= (first-stage-type query) :mbql.stage/native))
-(def ^:dynamic ^{:arglists '(^java.lang.String [^java.lang.String s])} *escape-alias-fn*
- "Function to use for escaping a unique alias when generating `:lib/desired-alias`."
- identity)
-
-(defn- unique-alias [original suffix]
- (->> (str original \_ suffix)
- *escape-alias-fn*
+(mu/defn ^:private escape-and-truncate :- :string
+ [database :- [:maybe ::lib.schema.metadata/database]
+ s :- :string]
+ (->> s
+ (lib.database.methods/escape-alias database)
;; truncate alias to 60 characters (actually 51 characters plus a hash).
truncate-alias))
-(mu/defn unique-name-generator :- [:=>
- [:cat ::lib.schema.common/non-blank-string]
- ::lib.schema.common/non-blank-string]
+(mu/defn ^:private unique-alias :- :string
+ [database :- [:maybe ::lib.schema.metadata/database]
+ original :- :string
+ suffix :- :string]
+ (->> (str original \_ suffix)
+ (escape-and-truncate database)))
+
+(mu/defn unique-name-generator :- [:function
+ ;; (f str) => unique-str
+ [:=>
+ [:cat :string]
+ ::lib.schema.common/non-blank-string]
+ ;; (f id str) => unique-str
+ [:=>
+ [:cat :any :string]
+ ::lib.schema.common/non-blank-string]]
"Create a new function with the signature
(f str) => str
+ or
+
+ (f id str) => str
+
That takes any sort of string identifier (e.g. a column alias or table/join alias) and returns a guaranteed-unique
name truncated to 60 characters (actually 51 characters plus a hash).
Optionally takes a list of names which are already defined, \"priming\" the generator with eg. all the column names
- that currently exist on a stage of the query."
- ([]
- (comp truncate-alias
- (mbql.u/unique-name-generator
- ;; unique by lower-case name, e.g. `NAME` and `name` => `NAME` and `name_2`
- ;;
- ;; some databases treat aliases as case-insensitive so make sure the generated aliases are unique regardless
- ;; of case
- :name-key-fn u/lower-case-en
- :unique-alias-fn unique-alias)))
-
- ([existing-names :- [:sequential :string]]
- (let [f (unique-name-generator)]
+ that currently exist on a stage of the query.
+
+ The two-arity version of the returned function can be used for idempotence. See docstring
+ for [[metabase.legacy-mbql.util/unique-name-generator]] for more information."
+ ([metadata-provider :- [:maybe ::lib.schema.metadata/metadata-provider]]
+ (let [database (some-> metadata-provider lib.metadata.protocols/database)
+ uniqify (mbql.u/unique-name-generator
+ ;; unique by lower-case name, e.g. `NAME` and `name` => `NAME` and `name_2`
+ ;;
+ ;; some databases treat aliases as case-insensitive so make sure the generated aliases are
+ ;; unique regardless of case
+ :name-key-fn u/lower-case-en
+ :unique-alias-fn (partial unique-alias database))]
+ ;; I know we could just use `comp` here but it gets really hard to figure out where it's coming from when you're
+ ;; debugging things; a named function like this makes it clear where this function came from
+ (fn unique-name-generator-fn
+ ([s]
+ (->> s
+ (escape-and-truncate database)
+ uniqify
+ truncate-alias))
+ ([id s]
+ (->> s
+ (escape-and-truncate database)
+ (uniqify id)
+ truncate-alias)))))
+
+ ([metadata-provider :- [:maybe ::lib.schema.metadata/metadata-provider]
+ existing-names :- [:sequential :string]]
+ (let [f (unique-name-generator metadata-provider)]
(doseq [existing existing-names]
(f existing))
f)))
diff --git a/src/metabase/query_processor/middleware/add_dimension_projections.clj b/src/metabase/query_processor/middleware/add_dimension_projections.clj
index 2ed14c5aba5..37de601281b 100644
--- a/src/metabase/query_processor/middleware/add_dimension_projections.clj
+++ b/src/metabase/query_processor/middleware/add_dimension_projections.clj
@@ -37,6 +37,7 @@
[metabase.lib.metadata.protocols :as lib.metadata.protocols]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
[metabase.query-processor.store :as qp.store]
[metabase.util :as u]
@@ -97,10 +98,7 @@
[fields :- [:maybe [:sequential mbql.s/Field]]]
(when-let [field-id->remapping-dimension (fields->field-id->remapping-dimension fields)]
;; Reconstruct how we uniquify names in [[metabase.query-processor.middleware.annotate]]
- ;;
- ;; Not sure this isn't broken. Probably better to have [[metabase.query-processor.util.add-alias-info]] do the name
- ;; deduplication instead.
- (let [name-generator (mbql.u/unique-name-generator)
+ (let [name-generator (lib.util/unique-name-generator (qp.store/metadata-provider))
unique-name (fn [field-id]
(assert (pos-int? field-id) (str "Invalid Field ID: " (pr-str field-id)))
(let [field (lib.metadata/field (qp.store/metadata-provider) field-id)]
diff --git a/src/metabase/query_processor/middleware/annotate.clj b/src/metabase/query_processor/middleware/annotate.clj
index 5a259076dda..1bd1efa8ede 100644
--- a/src/metabase/query_processor/middleware/annotate.clj
+++ b/src/metabase/query_processor/middleware/annotate.clj
@@ -14,6 +14,7 @@
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.calculation :as lib.metadata.calculation]
[metabase.lib.schema.common :as lib.schema.common]
+ [metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
[metabase.models.humanization :as humanization]
[metabase.query-processor.error-type :as qp.error-type]
@@ -78,7 +79,7 @@
:type qp.error-type/qp}))))))
(defn- annotate-native-cols [cols]
- (let [unique-name-fn (mbql.u/unique-name-generator)]
+ (let [unique-name-fn (lib.util/unique-name-generator (qp.store/metadata-provider))]
(vec (for [{col-name :name, base-type :base_type, :as driver-col-metadata} cols]
(let [col-name (name col-name)]
(merge
diff --git a/src/metabase/query_processor/middleware/cumulative_aggregations.clj b/src/metabase/query_processor/middleware/cumulative_aggregations.clj
index a6c7e11dd93..a18470c6e03 100644
--- a/src/metabase/query_processor/middleware/cumulative_aggregations.clj
+++ b/src/metabase/query_processor/middleware/cumulative_aggregations.clj
@@ -3,17 +3,19 @@
is only used for drivers that do not have native implementations of `:window-functions/cumulative`; see the driver
changelog for 0.50.0 for more information.
- For queries with more than one breakout, we reset the totals every time breakouts other than the last one change, e.g.
-
- ;; city date count cumulative_count
- LBC 2024-01-01 10 10
- LBC 2024-01-02 2 12
- LBC 2024-01-02 4 16
- SF 2024-01-01 3 3
- SF 2024-01-01 1 4
- SF 2024-01-02 2 6
-
- Rather than doing a cumulative sum across the entire set of query results -- see #2862 for more information."
+ For queries with more than one breakout, we reset the totals every time breakouts other than the first one change,
+ e.g.
+
+ date city count cumulative_count
+ 2024-01-01 LBC 10 10
+ 2024-01-02 LBC 2 12
+ 2024-01-02 LBC 4 16
+ 2024-01-01 SF 3 3
+ 2024-01-01 SF 1 4
+ 2024-01-02 SF 2 6
+
+ Rather than doing a cumulative sum across the entire set of query results -- see #2862 and #42003 for more
+ information."
(:require
[metabase.driver :as driver]
[metabase.legacy-mbql.schema :as mbql.s]
@@ -70,38 +72,51 @@
;;;; Post-processing
-(defn- partition-values [num-breakouts row]
- (when (> num-breakouts 1)
- (take (dec num-breakouts) row)))
+(defn- partition-key
+ "The values to partition the cumulative aggregation accumulation by (the equivalent of SQL `PARTITION BY` in a window
+ function). We partition by all breakouts other than the first. See #2862, #42003, and the docstring
+ for [[metabase.query-processor.middleware.cumulative-aggregations]] for more info."
+ [num-breakouts row]
+ ;; breakouts are always the first results returned. Return all breakouts except the first.
+ (when (pos? num-breakouts)
+ (subvec (vec row) 1 num-breakouts)))
-(defn- add-values-from-last-row
- "Update values in `row` by adding values from `last-row` for a set of specified indexes.
+(defn- add-values-from-last-partition-fn
+ "Create a stateful function that can add values from the previous row for each partition for a set of specified
+ indexes.
- ((add-values-from-last-row-fn 0) #{0} [100 200] [50 60]) ; -> [150 60]
+ (let [f (add-values-from-last-partition-fn 0 #{1})]
+ (f [100 200]) ; => [100 200]
+ (f [50 60])) ; => [50 260]
We need to reset the totals every time breakouts other than the last change values --
see [[metabase.query-processor.middleware.cumulative-aggregations]] docstring for more info."
- [num-breakouts indexes-to-sum last-row row]
- (if (or (not last-row)
- (not= (partition-values num-breakouts last-row)
- (partition-values num-breakouts row)))
- row
- (reduce (fn [row index]
- (update row index (partial (fnil + 0 0) (nth last-row index))))
- (vec row)
- indexes-to-sum)))
+ [num-breakouts indexes-to-sum]
+ (let [partition->last-row (volatile! nil)]
+ (fn [row]
+ (let [k (partition-key num-breakouts row)
+ last-row (get @partition->last-row k)
+ row' (if last-row
+ (reduce (fn [row index]
+ (update row index (partial (fnil + 0 0) (nth last-row index))))
+ (vec row)
+ indexes-to-sum)
+ row)]
+ ;; save the updated row for this partition key.
+ (vswap! partition->last-row assoc k row')
+ ;; now return the updated new row.
+ row'))))
(defn- cumulative-ags-xform [num-breakouts replaced-indexes rf]
{:pre [(fn? rf)]}
- (let [last-row (volatile! nil)]
+ (let [add-values-from-last-partition (add-values-from-last-partition-fn num-breakouts replaced-indexes)]
(fn
([] (rf))
([result] (rf result))
([result row]
- (let [row' (add-values-from-last-row num-breakouts replaced-indexes @last-row row)]
- (vreset! last-row row')
+ (let [row' (add-values-from-last-partition row)]
(rf result row'))))))
(defn sum-cumulative-aggregation-columns
diff --git a/src/metabase/query_processor/middleware/escape_join_aliases.clj b/src/metabase/query_processor/middleware/escape_join_aliases.clj
index 5c6f9faf41f..de088aeee4e 100644
--- a/src/metabase/query_processor/middleware/escape_join_aliases.clj
+++ b/src/metabase/query_processor/middleware/escape_join_aliases.clj
@@ -8,8 +8,9 @@
(:require
[clojure.set :as set]
[metabase.driver :as driver]
- [metabase.legacy-mbql.util :as mbql.u]
+ [metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
+ [metabase.query-processor.store :as qp.store]
[metabase.util :as u]
[metabase.util.log :as log]))
@@ -19,13 +20,7 @@
(driver/escape-alias driver join-alias))
(defn- driver->escape-fn [driver]
- (comp (mbql.u/unique-name-generator
- ;; some databases treat aliases as case-insensitive so make sure the generated aliases
- ;; are unique regardless of case
- :name-key-fn u/lower-case-en
- ;; uniqified aliases needs to be escaped again just in case
- :unique-alias-fn (fn [original suffix]
- (escape-alias driver (str original \_ suffix))))
+ (comp (lib.util/unique-name-generator (qp.store/metadata-provider))
(partial escape-alias driver)))
(defn- add-escaped-aliases
diff --git a/src/metabase/query_processor/setup.clj b/src/metabase/query_processor/setup.clj
index 028ddce01e2..63312e36b4c 100644
--- a/src/metabase/query_processor/setup.clj
+++ b/src/metabase/query_processor/setup.clj
@@ -145,15 +145,6 @@
(driver/with-driver driver
(f query))))))
-(mu/defn ^:private do-with-escape-alias-fn :- fn?
- [f :- [:=> [:cat ::qp.schema/query] :any]]
- (fn [query]
- (if-not (identical? lib.util/*escape-alias-fn* identity)
- (f query)
- (binding [lib.util/*escape-alias-fn* (fn [s]
- (driver/escape-alias driver/*driver* s))]
- (f query)))))
-
(mu/defn ^:private do-with-database-local-settings :- fn?
[f :- [:=> [:cat ::qp.schema/query] :any]]
(fn [query]
@@ -192,7 +183,6 @@
[#'do-with-canceled-chan
#'do-with-database-local-settings
#'do-with-driver
- #'do-with-escape-alias-fn
#'do-with-metadata-provider
#'do-with-resolved-database])
;;; ↑↑↑ SETUP MIDDLEWARE ↑↑↑ happens from BOTTOM to TOP e.g. [[do-with-resolved-database]] is the first to do its thing
diff --git a/src/metabase/query_processor/util/add_alias_info.clj b/src/metabase/query_processor/util/add_alias_info.clj
index c26700a356c..a8a61fda251 100644
--- a/src/metabase/query_processor/util/add_alias_info.clj
+++ b/src/metabase/query_processor/util/add_alias_info.clj
@@ -55,10 +55,11 @@
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.schema.common :as lib.schema.common]
[metabase.lib.schema.id :as lib.schema.id]
+ [metabase.lib.schema.metadata :as lib.schema.metadata]
+ [metabase.lib.util :as lib.util]
[metabase.lib.util.match :as lib.util.match]
[metabase.query-processor.error-type :as qp.error-type]
[metabase.query-processor.store :as qp.store]
- [metabase.util :as u]
[metabase.util.i18n :refer [trs tru]]
[metabase.util.malli :as mu]))
@@ -76,16 +77,7 @@
To return a uniquified version of `original-alias`. Memoized by `position`, so duplicate calls will result in the
same unique alias."
[]
- (let [unique-name-fn (mbql.u/unique-name-generator
- ;; some databases treat aliases as case-insensitive so make sure the generated aliases are
- ;; unique regardless of case
- :name-key-fn u/lower-case-en
- ;; TODO -- we should probably limit the length somehow like we do in
- ;; [[metabase.query-processor.middleware.add-implicit-joins/join-alias]], and also update this
- ;; function and that one to append a short suffix if we are limited by length. See also
- ;; [[driver/escape-alias]]
- :unique-alias-fn (fn [original suffix]
- (driver/escape-alias driver/*driver* (str original \_ suffix))))]
+ (let [unique-name-fn (lib.util/unique-name-generator (qp.store/metadata-provider))]
(fn unique-alias-fn [position original-alias]
{:pre (string? original-alias)}
(unique-name-fn position (driver/escape-alias driver/*driver* original-alias)))))
@@ -175,7 +167,7 @@
(when join-alias
((this-level-join-aliases inner-query) join-alias)))
-(mu/defn ^:private field-instance :- [:maybe lib.metadata/ColumnMetadata]
+(mu/defn ^:private field-instance :- [:maybe ::lib.schema.metadata/column]
[[_ id-or-name :as _field-clause] :- mbql.s/field]
(when (integer? id-or-name)
(lib.metadata/field (qp.store/metadata-provider) id-or-name)))
@@ -325,7 +317,7 @@
(mu/defmethod field-reference-mlv2 ::driver/driver
[driver :- :keyword
- field :- lib.metadata/ColumnMetadata]
+ field :- ::lib.schema.metadata/column]
#_{:clj-kondo/ignore [:deprecated-var]}
(if (get-method field-reference driver)
(do
diff --git a/src/metabase/query_processor/util/transformations/nest_breakouts.clj b/src/metabase/query_processor/util/transformations/nest_breakouts.clj
index b49aea09778..7d8f3e3c69f 100644
--- a/src/metabase/query_processor/util/transformations/nest_breakouts.clj
+++ b/src/metabase/query_processor/util/transformations/nest_breakouts.clj
@@ -54,7 +54,6 @@
;; for other columns: remove temporal type, it should be nil anyway but remove it to
;; be safe.
nil))
- (lib/with-join-alias nil)
(lib/with-binning nil)))
(mu/defn ^:private update-second-stage-refs :- ::lib.schema/stage
diff --git a/test/metabase/lib/util_test.cljc b/test/metabase/lib/util_test.cljc
index f60fec1dfae..b462d4cac3c 100644
--- a/test/metabase/lib/util_test.cljc
+++ b/test/metabase/lib/util_test.cljc
@@ -5,6 +5,7 @@
[clojure.test :refer [are deftest is testing]]
[metabase.lib.core :as lib]
[metabase.lib.test-metadata :as meta]
+ [metabase.lib.test-util :as lib.tu]
[metabase.lib.util :as lib.util]))
#?(:cljs
@@ -298,7 +299,7 @@
(truncate-alias s max-bytes)))))))
(deftest ^:parallel unique-name-generator-test
- (let [unique-name-fn (lib.util/unique-name-generator)]
+ (let [unique-name-fn (lib.util/unique-name-generator meta/metadata-provider)]
(is (= "wow"
(unique-name-fn "wow")))
(is (= "wow_2"
@@ -309,9 +310,30 @@
(testing "should truncate long names"
(is (= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY_2dc86ef1"
(unique-name-fn "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789")))
- (is (= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY_1380b38f"
+ (is (= "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXY_fc11882d"
(unique-name-fn "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"))))))
+(deftest ^:parallel unique-name-generator-idempotence-test
+ (testing "idempotence (2-arity calls to generated function)"
+ (let [unique-name (lib.util/unique-name-generator meta/metadata-provider)]
+ (is (= ["A" "B" "A" "A_2" "A_2"]
+ [(unique-name :x "A")
+ (unique-name :x "B")
+ (unique-name :x "A")
+ (unique-name :y "A")
+ (unique-name :y "A")])))))
+
+(deftest ^:parallel unique-name-use-database-methods-test
+ (testing "Should use database :lib/methods"
+ (let [metadata-provider (lib.tu/merged-mock-metadata-provider
+ meta/metadata-provider
+ {:database {:lib/methods {:escape-alias #(lib.util/truncate-alias % 15)}}})
+ unique-name (lib.util/unique-name-generator metadata-provider)]
+ (is (= "catego_ef520013"
+ (unique-name "categories__via__category_id__name")))
+ (is (= "catego_ec940c72"
+ (unique-name "categories__via__category_id__name"))))))
+
(deftest ^:parallel strip-id-test
(are [exp in] (= exp (lib.util/strip-id in))
"foo" "foo"
diff --git a/test/metabase/query_processor/middleware/annotate_test.clj b/test/metabase/query_processor/middleware/annotate_test.clj
index 3016c92f1fd..c69613bd367 100644
--- a/test/metabase/query_processor/middleware/annotate_test.clj
+++ b/test/metabase/query_processor/middleware/annotate_test.clj
@@ -28,26 +28,33 @@
(testing "should still infer types even if the initial value(s) are `nil` (#4256, #6924)"
(is (= [:type/Integer]
(transduce identity (#'annotate/base-type-inferer {:cols [{}]})
- (concat (repeat 1000 [nil]) [[1] [2]])))))
+ (concat (repeat 1000 [nil]) [[1] [2]])))))))
+(deftest ^:parallel native-column-info-test-2
+ (testing "native column info"
(testing "should use default `base_type` of `type/*` if there are no non-nil values in the sample"
(is (= [:type/*]
(transduce identity (#'annotate/base-type-inferer {:cols [{}]})
- [[nil]]))))
+ [[nil]]))))))
+(deftest ^:parallel native-column-info-test-3
+ (testing "native column info"
(testing "should attempt to infer better base type if driver returns :type/* (#12150)"
;; `merged-column-info` handles merging info returned by driver & inferred by annotate
(is (= [:type/Integer]
(transduce identity (#'annotate/base-type-inferer {:cols [{:base_type :type/*}]})
- [[1] [2] [nil] [3]]))))
+ [[1] [2] [nil] [3]]))))))
+(deftest ^:parallel native-column-info-test-4
+ (testing "native column info"
(testing "should disambiguate duplicate names"
- (is (= [{:name "a", :display_name "a", :base_type :type/Integer, :source :native, :field_ref [:field "a" {:base-type :type/Integer}]}
- {:name "a", :display_name "a", :base_type :type/Integer, :source :native, :field_ref [:field "a_2" {:base-type :type/Integer}]}]
- (annotate/column-info
- {:type :native}
- {:cols [{:name "a" :base_type :type/Integer} {:name "a" :base_type :type/Integer}]
- :rows [[1 nil]]}))))))
+ (qp.store/with-metadata-provider meta/metadata-provider
+ (is (= [{:name "a", :display_name "a", :base_type :type/Integer, :source :native, :field_ref [:field "a" {:base-type :type/Integer}]}
+ {:name "a", :display_name "a", :base_type :type/Integer, :source :native, :field_ref [:field "a_2" {:base-type :type/Integer}]}]
+ (annotate/column-info
+ {:type :native}
+ {:cols [{:name "a" :base_type :type/Integer} {:name "a" :base_type :type/Integer}]
+ :rows [[1 nil]]})))))))
(deftest ^:parallel col-info-field-ids-test
(testing {:base-type "make sure columns are comming back the way we'd expect for :field clauses"}
diff --git a/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj b/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj
index 88e94eb4710..f0021b274a1 100644
--- a/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj
+++ b/test/metabase/query_processor/middleware/cumulative_aggregations_test.clj
@@ -6,23 +6,34 @@
[metabase.query-processor.middleware.cumulative-aggregations :as qp.cumulative-aggregations]
[metabase.query-processor.store :as qp.store]))
-(deftest ^:parallel add-values-from-last-row-test
- (are [expected indecies] (= expected
- (#'qp.cumulative-aggregations/add-values-from-last-row 0 indecies [1 2 3] [1 2 3]))
+(deftest ^:parallel add-values-from-last-partition-test
+ (are [expected indexes] (= expected
+ (let [f (#'qp.cumulative-aggregations/add-values-from-last-partition-fn 0 indexes)]
+ (f [1 2 3])
+ (f [1 2 3])))
[1 2 3] #{}
[2 2 3] #{0}
[2 4 3] #{0 1}
- [1 4 6] #{1 2})
+ [1 4 6] #{1 2}))
- (is (thrown?
- IndexOutOfBoundsException
- (#'qp.cumulative-aggregations/add-values-from-last-row 0 #{4} [1 2 3] [1 2 3]))
- "should throw an Exception if index is out of bounds")
+(deftest ^:parallel add-values-from-last-partition-out-of-bounds-test
+ (let [f (#'qp.cumulative-aggregations/add-values-from-last-partition-fn 0 #{4})]
+ (is (thrown?
+ IndexOutOfBoundsException
+ (do
+ (f [1 2 3])
+ (f [1 2 3])))
+ "should throw an Exception if index is out of bounds")))
+(deftest ^:parallel add-values-from-last-partition-nils-test
(testing "Do we handle nils correctly"
- (is (= [1] (#'qp.cumulative-aggregations/add-values-from-last-row 0 #{0} [nil] [1])))
- (is (= [0] (#'qp.cumulative-aggregations/add-values-from-last-row 0 #{0} [nil] [nil])))
- (is (= [1] (#'qp.cumulative-aggregations/add-values-from-last-row 0 #{0} [1] [nil])))))
+ (are [row-1 row-2 expected] (= expected
+ (let [f (#'qp.cumulative-aggregations/add-values-from-last-partition-fn 0 #{0})]
+ (f row-1)
+ (f row-2)))
+ [nil] [1] [1]
+ [nil] [nil] [0]
+ [1] [nil] [1])))
(deftest ^:parallel diff-indicies-test
(testing "collections are the same"
@@ -42,17 +53,17 @@
(deftest ^:parallel transduce-results-test
(testing "Transducing result rows"
(let [rows [[0] [1] [2] [3] [4] [5] [6] [7] [8] [9]]]
- (testing "0/1 indecies"
+ (testing "0/1 indexes"
(is (= rows
(sum-rows #{} rows))))
- (testing "1/1 indecies"
+ (testing "1/1 indexes"
(is (= [[0] [1] [3] [6] [10] [15] [21] [28] [36] [45]]
(sum-rows #{0} rows)))))
(let [rows [[0 0] [1 1] [2 2] [3 3] [4 4] [5 5] [6 6] [7 7] [8 8] [9 9]]]
- (testing "1/2 indecies"
+ (testing "1/2 indexes"
(is (= [[0 0] [1 1] [3 2] [6 3] [10 4] [15 5] [21 6] [28 7] [36 8] [45 9]]
(sum-rows #{0} rows))))
- (testing "2/2 indecies"
+ (testing "2/2 indexes"
(is (= [[0 0] [1 1] [3 3] [6 6] [10 10] [15 15] [21 21] [28 28] [36 36] [45 45]]
(sum-rows #{0 1} rows)))))
(testing "sum-rows should still work if rows are lists"
@@ -121,27 +132,49 @@
:aggregation [[:+ [:cum-count] 1]]}}))))))
(deftest ^:parallel multiple-breakouts-reset-counts-test
- (testing "Multiple breakouts: reset counts after breakouts other than last get new values (#2862)"
- (let [rows [["Long Beach" #t "2016-04-01" 2]
- ["Long Beach" #t "2016-04-03" 4] ; 2 + 4 = 6
- ["Long Beach" #t "2016-04-03" 6] ; 6 + 6 = 12
- ["Long Beach" #t "2016-04-06" 8] ; 12 + 8 = 20
- ["San Francisco" #t "2016-04-01" 10] ; RESET VALUES HERE!
- ["San Francisco" #t "2016-04-01" 20] ; 10 + 20 = 30
- ["San Francisco" #t "2016-04-02" 30] ; 30 + 30 = 60
- ["San Francisco" #t "2016-04-04" 40]] ; 60 + 40 = 100
+ (testing "Multiple breakouts: reset counts after breakouts other than first get new values (#2862, #42003)"
+ (let [rows [[#t "2016-04-01" "Long Beach" 2] ; [LB] 0 + 2 => 2
+ [#t "2016-04-01" "San Francisco" 10] ; [SF] 0 + 10 => 10
+ [#t "2016-04-02" "San Francisco" 30] ; [SF] 10 + 30 => 40
+ [#t "2016-04-03" "Long Beach" 4] ; [LB] 2 + 4 => 6
+ [#t "2016-04-04" "San Francisco" 40] ; [SF] 40 + 40 => 80
+ [#t "2016-04-06" "Long Beach" 8]] ; [LB] 6 + 8 => 14
rff (qp.cumulative-aggregations/sum-cumulative-aggregation-columns
{::qp.cumulative-aggregations/replaced-indexes #{2}
:query {:breakout [:a :b]}}
(fn [_metadata]
conj))
rf (rff nil)]
- (is (= [["Long Beach" #t "2016-04-01" 2]
- ["Long Beach" #t "2016-04-03" 6]
- ["Long Beach" #t "2016-04-03" 12]
- ["Long Beach" #t "2016-04-06" 20]
- ["San Francisco" #t "2016-04-01" 10]
- ["San Francisco" #t "2016-04-01" 30]
- ["San Francisco" #t "2016-04-02" 60]
- ["San Francisco" #t "2016-04-04" 100]]
+ (is (= [[#t "2016-04-01" "Long Beach" 2] ; [LB] 0 + 2 => 2
+ [#t "2016-04-01" "San Francisco" 10] ; [SF] 0 + 10 => 10
+ [#t "2016-04-02" "San Francisco" 40] ; [SF] 10 + 30 => 40
+ [#t "2016-04-03" "Long Beach" 6] ; [LB] 2 + 4 => 6
+ [#t "2016-04-04" "San Francisco" 80] ; [SF] 40 + 40 => 80
+ [#t "2016-04-06" "Long Beach" 14]] ; [LB] 6 + 8 => 14
+ (reduce rf [] rows))))))
+
+(deftest ^:parallel multiple-breakouts-reset-counts-test-2
+ (testing "3 breakouts: reset counts after breakouts other than first get new values (#2862, #42003)"
+ (let [rows [[#t "2016-04-01" "Long Beach" "LB" 2] ; [LB] 0 + 2 => 2
+ [#t "2016-04-01" "San Francisco" "SF" 10] ; [SF] 0 + 10 => 10
+ [#t "2016-04-02" "San Francisco" "SF" 30] ; [SF] 10 + 30 => 40
+ [#t "2016-04-03" "Long Beach" "LB" 4] ; [LB] 2 + 4 => 6
+ [#t "2016-04-03" "Long Beach" "LBC" 4] ; [LBC] 0 + 4 => 4
+ [#t "2016-04-04" "San Francisco" "SF" 40] ; [SF] 40 + 40 => 80
+ [#t "2016-04-06" "Long Beach" "LB" 8] ; [LB] 6 + 8 => 14
+ [#t "2016-04-06" "Long Beach" "LBC" 8]] ; [LBC] 4 + 8 => 12
+ rff (qp.cumulative-aggregations/sum-cumulative-aggregation-columns
+ {::qp.cumulative-aggregations/replaced-indexes #{3}
+ :query {:breakout [:a :b :c]}}
+ (fn [_metadata]
+ conj))
+ rf (rff nil)]
+ (is (= [[#t "2016-04-01" "Long Beach" "LB" 2] ; [LB] 0 + 2 => 2
+ [#t "2016-04-01" "San Francisco" "SF" 10] ; [SF] 0 + 10 => 10
+ [#t "2016-04-02" "San Francisco" "SF" 40] ; [SF] 10 + 30 => 40
+ [#t "2016-04-03" "Long Beach" "LB" 6] ; [LB] 2 + 4 => 6
+ [#t "2016-04-03" "Long Beach" "LBC" 4] ; [LBC] 0 + 4 => 4
+ [#t "2016-04-04" "San Francisco" "SF" 80] ; [SF] 40 + 40 => 80
+ [#t "2016-04-06" "Long Beach" "LB" 14] ; [LB] 6 + 8 => 14
+ [#t "2016-04-06" "Long Beach" "LBC" 12]] ; [LBC] 4 + 8 => 12
(reduce rf [] rows))))))
diff --git a/test/metabase/query_processor/middleware/escape_join_aliases_test.clj b/test/metabase/query_processor/middleware/escape_join_aliases_test.clj
index dc554ac260b..1d00d193ea2 100644
--- a/test/metabase/query_processor/middleware/escape_join_aliases_test.clj
+++ b/test/metabase/query_processor/middleware/escape_join_aliases_test.clj
@@ -4,8 +4,11 @@
[flatland.ordered.map :as ordered-map]
[metabase.driver :as driver]
[metabase.driver.impl :as driver.impl]
+ [metabase.lib.test-metadata :as meta]
+ [metabase.lib.test-util :as lib.tu]
[metabase.lib.util.match :as lib.util.match]
[metabase.query-processor.middleware.escape-join-aliases :as escape]
+ [metabase.query-processor.store :as qp.store]
[metabase.util :as u]))
(driver/register! ::custom-escape :abstract? true)
@@ -14,6 +17,14 @@
[_driver s]
(driver.impl/truncate-alias s 12))
+(defn- do-with-metadata-provider [thunk]
+ (if (qp.store/initialized?)
+ (thunk)
+ (qp.store/with-metadata-provider (lib.tu/merged-mock-metadata-provider
+ meta/metadata-provider
+ {:database {:lib/methods {:escape-alias #(driver/escape-alias driver/*driver* %)}}})
+ (thunk))))
+
;;; the tests below are tests for the individual sub-steps of the middleware.
(deftest ^:parallel replace-original-aliases-with-escaped-aliases-test
@@ -37,10 +48,16 @@
{"Products" "Products", "Q2" "Q2"}}}))))
(defn- add-escaped-aliases-h2 [query]
- (#'escape/add-escaped-aliases query (#'escape/driver->escape-fn :h2)))
+ (driver/with-driver :h2
+ (do-with-metadata-provider
+ (fn []
+ (#'escape/add-escaped-aliases query (#'escape/driver->escape-fn :h2))))))
(defn- add-escaped-aliases-custom-escape [query]
- (#'escape/add-escaped-aliases query (#'escape/driver->escape-fn ::custom-escape)))
+ (driver/with-driver ::custom-escape
+ (do-with-metadata-provider
+ (fn []
+ (#'escape/add-escaped-aliases query (#'escape/driver->escape-fn ::custom-escape))))))
;;; the tests below test what a query should look like after each sub-step
@@ -466,6 +483,11 @@
;;; these are e2e tests
+(defn- escape-join-aliases [query]
+ (do-with-metadata-provider
+ (fn []
+ (escape/escape-join-aliases query))))
+
(deftest ^:parallel deduplicate-alias-names-test
(testing "Should ensure all join aliases are unique, ignoring case"
;; some Databases treat table/subquery aliases as case-insensitive and thus `Cat` and `cat` would be considered the
@@ -484,7 +506,7 @@
[:field 4 {:join-alias "Cat"}]
[:field 4 {:join-alias "cat_2"}]]}
:info {:alias/escaped->original {"cat_2" "cat"}}}
- (escape/escape-join-aliases
+ (escape-join-aliases
{:database 1
:type :query
:query {:source-table 1
@@ -496,7 +518,9 @@
:condition [:= [:field 3 nil] [:field 4 {:join-alias "cat"}]]}]
:fields [[:field 3 nil]
[:field 4 {:join-alias "Cat"}]
- [:field 4 {:join-alias "cat"}]]}})))))
+ [:field 4 {:join-alias "cat"}]]}}))))))
+
+(deftest ^:parallel deduplicate-alias-names-test-2
(testing "no need to include alias info if they have not changed"
(driver/with-driver :h2
(let [query {:database 1
@@ -510,7 +534,7 @@
:fields [[:field 3 nil]
[:field 4 {:join-alias "Cat"}]
[:field 4 {:join-alias "cat"}]]}}
- q' (escape/escape-join-aliases query)]
+ q' (escape-join-aliases query)]
(testing "No need for a map with identical mapping"
(is (not (contains? (:info q') :alias/escaped->original))))
(testing "aliases in the query remain the same"
@@ -541,7 +565,7 @@
:info {:alias/escaped->original {"ê°€_50a93035" "가나다ë¼ë§ˆ"
"012_68c4f033" "0123456789abcdef"}}}
(driver/with-driver ::custom-escape
- (escape/escape-join-aliases
+ (escape-join-aliases
{:database 1
:type :query
:query {:source-table 1
@@ -585,7 +609,7 @@
:order-by [[:asc [:field 6 {:join-alias "Products", :temporal-unit :month}]]]}
:info {:alias/escaped->original {"Products_2" "Products"}}}
(driver/with-driver :h2
- (escape/escape-join-aliases
+ (escape-join-aliases
{:query {:source-query {:source-table 1
:joins [{:source-table 2
:alias "Products"
@@ -635,7 +659,7 @@
[:field 6 {:join-alias "Q2", :temporal-unit :month}]]}]}
:info {:alias/escaped->original {"Products_2" "Products"}}}
(driver/with-driver :h2
- (escape/escape-join-aliases
+ (escape-join-aliases
{:query {:source-query {:source-table 1
:joins [{:source-table 2
:alias "Products"
@@ -668,7 +692,7 @@
[:field 5 {:join-alias "Products_2"}]]}]}]}
:info {:alias/escaped->original {"Products_2" "Products"}}}
(driver/with-driver :h2
- (escape/escape-join-aliases
+ (escape-join-aliases
{:query {:source-query {:source-table 1
:joins [{:source-table 2
:alias "Products"
@@ -693,7 +717,7 @@
:order-by [[:asc [:field 6 {:join-alias "Pro_acce5f27", :temporal-unit :month}]]]}
:info {:alias/escaped->original {"Pro_acce5f27" "Products_Long_Identifier"}}}
(driver/with-driver ::custom-escape
- (escape/escape-join-aliases
+ (escape-join-aliases
{:query {:source-query {:source-query {:source-table 1
:joins [{:source-table 2
:alias "Products_Long_Identifier"
diff --git a/test/metabase/query_processor/util/transformations/nest_breakouts_test.clj b/test/metabase/query_processor/util/transformations/nest_breakouts_test.clj
index 28f95c7408a..3cd6bd1a0e6 100644
--- a/test/metabase/query_processor/util/transformations/nest_breakouts_test.clj
+++ b/test/metabase/query_processor/util/transformations/nest_breakouts_test.clj
@@ -1,9 +1,14 @@
(ns metabase.query-processor.util.transformations.nest-breakouts-test
(:require
[clojure.test :refer :all]
+ [medley.core :as m]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
+ [metabase.lib.query :as lib.query]
[metabase.lib.test-metadata :as meta]
+ [metabase.lib.test-util :as lib.tu]
+ [metabase.lib.util :as lib.util]
+ [metabase.query-processor.preprocess :as qp.preprocess]
[metabase.query-processor.util.transformations.nest-breakouts :as nest-breakouts]))
(deftest ^:parallel cumulative-aggregation-with-filter-and-temporal-bucketed-breakout-test
@@ -72,3 +77,82 @@
[:cum-count {}]]]
:limit 3}]}
(nest-breakouts/nest-breakouts-in-stages-with-window-aggregation query)))))
+
+(defn- mock-escape-alias
+ "This is based on Oracle's implementation of [[metabase.driver/escape-alias]]."
+ [s]
+ (lib.util/truncate-alias s 30))
+
+(deftest ^:parallel escape-identifiers-correctly-test
+ (testing (str "Refs in the second stage need to get escaped using [[metabase.driver/escape-alias]] (indirectly via"
+ " [[metabase.lib.database.methods/escape-alias]]). Simulate Oracle behavior here, which truncates all"
+ " identifiers to 30 characters. Make sure `mock-escape-alias` gets used everywhere, including for"
+ " generating join aliases.")
+ (let [metadata-provider (lib.tu/merged-mock-metadata-provider
+ meta/metadata-provider
+ {:database {:lib/methods {:escape-alias mock-escape-alias}}
+ ;; mock the Oracle names for things since I want to verify that this behaves correctly
+ ;; specifically for Oracle.
+ :tables [{:id (meta/id :orders)
+ :schema "mb_test"
+ :name "test_data_orders"}
+ {:id (meta/id :products)
+ :schema "mb_test"
+ :name "test_data_products"}]
+ :fields [{:id (meta/id :orders :created-at), :name "created_at"}
+ {:id (meta/id :orders :id), :name "id"}
+ {:id (meta/id :orders :product-id), :name "product_id"}
+ {:id (meta/id :products :id), :name "id"}
+ {:id (meta/id :products :category), :name "category"}]})
+ orders (lib.metadata/table metadata-provider (meta/id :orders))
+ orders-created-at (lib.metadata/field metadata-provider (meta/id :orders :created-at))
+ orders-id (lib.metadata/field metadata-provider (meta/id :orders :id))
+ products-category (m/find-first (fn [col]
+ (= (:id col) (meta/id :products :category)))
+ (lib/visible-columns (lib/query metadata-provider orders)))
+ _ (assert (some? products-category))
+ query (-> (lib/query metadata-provider orders)
+ (lib/filter (lib/> orders-id 5000))
+ (lib/aggregate (lib/count))
+ (lib/aggregate (lib/cum-count))
+ (lib/breakout (lib/with-temporal-bucket orders-created-at :year))
+ (lib/breakout products-category))
+ preprocessed (lib.query/query metadata-provider (qp.preprocess/preprocess query))]
+ (is (=? {:stages [{;; join alias is escaped:
+ ;;
+ ;; (metabase.driver/escape-alias :oracle "test_data_products__via__product_id")
+ ;; =>
+ ;; "test_data_products__v_af2712b9"
+ :joins [{:alias "test_data_products__v_af2712b9"
+ :conditions [[:=
+ {}
+ [:field {} pos-int?]
+ [:field
+ {:join-alias "test_data_products__v_af2712b9"}
+ pos-int?]]]
+ :stages [{:lib/type :mbql.stage/mbql, :source-table pos-int?}]}]
+ :fields [[:field {} pos-int?]
+ [:field {:join-alias "test_data_products__v_af2712b9"} pos-int?]]
+ :filters [[:>
+ {}
+ [:field {} pos-int?]
+ [:value {} 5000]]]}
+ ;; new second stage. Source column names are escaped:
+ ;;
+ ;; (metabase.driver/escape-alias :oracle "test_data_products__v_af2712b9__category")
+ ;; =>
+ ;; "test_data_products__v_f48e965c"
+ {:breakout [[:field {} "created_at"]
+ [:field {:join-alias (symbol "nil #_\"key is not present.\"")
+ :source-field (symbol "nil #_\"key is not present.\"")}
+ "test_data_products__v_f48e965c"]]
+ :aggregation [[:count {:name "count"}]
+ [:cum-count {:name "count_2"}]]
+ :order-by [[:asc {} [:field {} "created_at"]]
+ [:asc {} [:field {:join-alias (symbol "nil #_\"key is not present.\"")
+ :source-field (symbol "nil #_\"key is not present.\"")}
+ "test_data_products__v_f48e965c"]]]}]
+ ;; unrelated TODO, but I think `:alias/escaped->original` should be a string -> string map not keyword ->
+ ;; string
+ :info {:alias/escaped->original {:test-data-products--v-af2712b9 "test_data_products__via__product_id"}}}
+ (nest-breakouts/nest-breakouts-in-stages-with-window-aggregation preprocessed))))))
diff --git a/test/metabase/query_processor_test/cumulative_aggregation_test.clj b/test/metabase/query_processor_test/cumulative_aggregation_test.clj
index 48dec6b4bb6..07c56ae524f 100644
--- a/test/metabase/query_processor_test/cumulative_aggregation_test.clj
+++ b/test/metabase/query_processor_test/cumulative_aggregation_test.clj
@@ -2,6 +2,7 @@
(:require
[clojure.test :refer :all]
[java-time.api :as t]
+ [medley.core :as m]
[metabase.lib.core :as lib]
[metabase.lib.metadata :as lib.metadata]
[metabase.lib.metadata.jvm :as lib.metadata.jvm]
@@ -160,42 +161,42 @@
(qp/process-query query)))))))))
(deftest ^:parallel cumulative-count-with-multiple-breakouts-test
- (testing "Should be PARTIONED BY first BREAKOUT and ORDERED BY the second BREAKOUT (#2862)"
+ (testing "Should be ORDERED BY first BREAKOUT and PARTITIONED BY the second BREAKOUT (#2862, #42003)"
(mt/test-drivers (mt/normal-drivers)
(let [query (-> (mt/mbql-query orders
{:aggregation [[:cum-count]]
- :breakout [!year.created_at !month.created_at]
+ :breakout [!month.created_at !year.created_at]
:limit 12})
(assoc-in [:middleware :format-rows?] false))]
(mt/with-native-query-testing-context query
- (is (= [[#t "2016-01-01" #t "2016-04-01" 1]
- [#t "2016-01-01" #t "2016-05-01" 20]
- [#t "2016-01-01" #t "2016-06-01" 57]
- [#t "2016-01-01" #t "2016-07-01" 121]
- [#t "2016-01-01" #t "2016-08-01" 200]
- [#t "2016-01-01" #t "2016-09-01" 292]
- [#t "2016-01-01" #t "2016-10-01" 429]
- [#t "2016-01-01" #t "2016-11-01" 579]
- [#t "2016-01-01" #t "2016-12-01" 744]
- [#t "2017-01-01" #t "2017-01-01" 205] ; <--- total should reset here, when first breakout changes
- [#t "2017-01-01" #t "2017-02-01" 411]
- [#t "2017-01-01" #t "2017-03-01" 667]]
+ (is (= [[#t "2016-04-01" #t "2016-01-01" 1]
+ [#t "2016-05-01" #t "2016-01-01" 20]
+ [#t "2016-06-01" #t "2016-01-01" 57]
+ [#t "2016-07-01" #t "2016-01-01" 121]
+ [#t "2016-08-01" #t "2016-01-01" 200]
+ [#t "2016-09-01" #t "2016-01-01" 292]
+ [#t "2016-10-01" #t "2016-01-01" 429]
+ [#t "2016-11-01" #t "2016-01-01" 579]
+ [#t "2016-12-01" #t "2016-01-01" 744]
+ [#t "2017-01-01" #t "2017-01-01" 205] ; <--- total should reset here, when second breakout changes
+ [#t "2017-02-01" #t "2017-01-01" 411]
+ [#t "2017-03-01" #t "2017-01-01" 667]]
(mt/formatted-rows [->local-date ->local-date int]
(qp/process-query query)))))))))
(deftest ^:parallel cumulative-count-with-three-breakouts-test
- (testing "Three breakouts: should be PARTIONED BY first and second BREAKOUTS and ORDERED BY the last BREAKOUT (#2862)"
+ (testing "Three breakouts: should be ORDERED BY first BREAKOUT and PARTITIONED BY second and third BREAKOUTS (#2862, #42003)"
(mt/test-drivers (mt/normal-drivers)
(let [query (-> (mt/mbql-query orders
{:aggregation [[:cum-count]]
- :breakout [!year.created_at !month.created_at !day.created_at]
+ :breakout [!day.created_at !year.created_at !month.created_at]
:limit 4})
(assoc-in [:middleware :format-rows?] false))]
(mt/with-native-query-testing-context query
- (is (= [[#t "2016-01-01" #t "2016-04-01" #t "2016-04-30" 1]
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-04" 1] ; <- count should reset here, when first two breakouts change
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-06" 2]
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-08" 3]]
+ (is (= [[#t "2016-04-30" #t "2016-01-01" #t "2016-04-01" 1]
+ [#t "2016-05-04" #t "2016-01-01" #t "2016-05-01" 1] ; <- count should reset here, when last two breakouts change
+ [#t "2016-05-06" #t "2016-01-01" #t "2016-05-01" 2]
+ [#t "2016-05-08" #t "2016-01-01" #t "2016-05-01" 3]]
(mt/formatted-rows [->local-date ->local-date ->local-date int]
(qp/process-query query)))))))))
@@ -233,44 +234,44 @@
(qp/process-query query))))))))))
(deftest ^:parallel cumulative-sum-with-multiple-breakouts-test
- (testing "Should be PARTIONED BY first BREAKOUT and ORDERED BY the second BREAKOUT (#2862)"
+ (testing "Should be ORDERED BY first BREAKOUT and PARTITIONED BY the second BREAKOUT (#2862, #42003)"
(mt/test-drivers (mt/normal-drivers)
(let [query (-> (mt/mbql-query orders
{:aggregation [[:cum-sum $total]]
- :breakout [!year.created_at !month.created_at]
+ :breakout [!month.created_at !year.created_at]
:limit 12})
(assoc-in [:middleware :format-rows?] false))]
(mt/with-native-query-testing-context query
;; you can sanity check these numbers by changing `:cum-sum` to `:sum` and adding them up manually
- (is (= [[#t "2016-01-01" #t "2016-04-01" 52.76]
- [#t "2016-01-01" #t "2016-05-01" 1318.49]
- [#t "2016-01-01" #t "2016-06-01" 3391.41]
- [#t "2016-01-01" #t "2016-07-01" 7126.13]
- [#t "2016-01-01" #t "2016-08-01" 12086.78]
- [#t "2016-01-01" #t "2016-09-01" 17458.87]
- [#t "2016-01-01" #t "2016-10-01" 25161.80]
- [#t "2016-01-01" #t "2016-11-01" 33088.49]
- [#t "2016-01-01" #t "2016-12-01" 42156.94]
- [#t "2017-01-01" #t "2017-01-01" 11094.77] ; <--- total should reset here, when first breakout changes
- [#t "2017-01-01" #t "2017-02-01" 22338.43]
- [#t "2017-01-01" #t "2017-03-01" 36454.11]]
+ (is (= [[#t "2016-04-01" #t "2016-01-01" 52.76]
+ [#t "2016-05-01" #t "2016-01-01" 1318.49]
+ [#t "2016-06-01" #t "2016-01-01" 3391.41]
+ [#t "2016-07-01" #t "2016-01-01" 7126.13]
+ [#t "2016-08-01" #t "2016-01-01" 12086.78]
+ [#t "2016-09-01" #t "2016-01-01" 17458.87]
+ [#t "2016-10-01" #t "2016-01-01" 25161.80]
+ [#t "2016-11-01" #t "2016-01-01" 33088.49]
+ [#t "2016-12-01" #t "2016-01-01" 42156.94]
+ [#t "2017-01-01" #t "2017-01-01" 11094.77] ; <--- total should reset here, when second breakout changes
+ [#t "2017-02-01" #t "2017-01-01" 22338.43]
+ [#t "2017-03-01" #t "2017-01-01" 36454.11]]
(mt/formatted-rows [->local-date ->local-date 2.0]
(qp/process-query query)))))))))
(deftest ^:parallel cumulative-sum-with-three-breakouts-test
- (testing "Three breakouts: should be PARTIONED BY first and second BREAKOUTS and ORDERED BY the last BREAKOUT (#2862)"
+ (testing "Three breakouts: should be ORDERED BY first BREAKOUT and PARTITIONED BY last two BREAKOUTS (#2862, #42003)"
(mt/test-drivers (mt/normal-drivers)
(let [query (-> (mt/mbql-query orders
{:aggregation [[:cum-sum $total]]
- :breakout [!year.created_at !month.created_at !day.created_at]
+ :breakout [!day.created_at !year.created_at !month.created_at]
:limit 4})
(assoc-in [:middleware :format-rows?] false))]
(mt/with-native-query-testing-context query
;; you can sanity check these numbers by changing `:cum-sum` to `:sum` and adding them up manually.
- (is (= [[#t "2016-01-01" #t "2016-04-01" #t "2016-04-30" 52.76]
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-04" 98.78] ; <-- total should reset here, when first two breakouts change
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-06" 186.07]
- [#t "2016-01-01" #t "2016-05-01" #t "2016-05-08" 270.94]]
+ (is (= [[#t "2016-04-30" #t "2016-01-01" #t "2016-04-01" 52.76]
+ [#t "2016-05-04" #t "2016-01-01" #t "2016-05-01" 98.78] ; <-- total should reset here, when last two breakouts change
+ [#t "2016-05-06" #t "2016-01-01" #t "2016-05-01" 186.07]
+ [#t "2016-05-08" #t "2016-01-01" #t "2016-05-01" 270.94]]
(mt/formatted-rows [->local-date ->local-date ->local-date 2.0]
(qp/process-query query)))))))))
@@ -347,25 +348,36 @@
(qp/process-query query))))))))
(deftest ^:parallel cumulative-aggregation-with-filter-and-temporal-bucketed-breakout-test
- (testing "Query with a filter and a temporally bucketed breakout should work (#41791)"
- (let [metadata-provider (lib.metadata.jvm/application-database-metadata-provider (mt/id))
- orders (lib.metadata/table metadata-provider (mt/id :orders))
- orders-quantity (lib.metadata/field metadata-provider (mt/id :orders :quantity))
- orders-created-at (lib.metadata/field metadata-provider (mt/id :orders :created_at))
- orders-id (lib.metadata/field metadata-provider (mt/id :orders :id))
- query (-> (lib/query metadata-provider orders)
- (lib/filter (lib/> orders-id 5000))
- (lib/aggregate (lib/cum-count))
- (lib/breakout (lib/with-temporal-bucket orders-created-at :month))
- (lib/breakout orders-quantity)
- (lib/limit 5)
- (assoc-in [:middleware :format-rows?] false))]
- (mt/with-native-query-testing-context query
- (is (= [[#t "2016-04-01" 2 1]
- [#t "2016-05-01" 2 1]
- [#t "2016-05-01" 3 4]
- [#t "2016-05-01" 4 10]
- [#t "2016-05-01" 5 17]]
- (mt/formatted-rows
- [->local-date int int]
- (qp/process-query query))))))))
+ (mt/test-drivers (mt/normal-drivers-with-feature :left-join :window-functions/cumulative)
+ (testing "Query with a filter and a temporally bucketed breakout should work (#41791)"
+ (let [metadata-provider (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+ orders (lib.metadata/table metadata-provider (mt/id :orders))
+ orders-created-at (lib.metadata/field metadata-provider (mt/id :orders :created_at))
+ orders-id (lib.metadata/field metadata-provider (mt/id :orders :id))
+ products-category (m/find-first (fn [col]
+ (= (:id col) (mt/id :products :category)))
+ (lib/visible-columns (lib/query metadata-provider orders)))
+ _ (assert (some? products-category))
+ query (-> (lib/query metadata-provider orders)
+ (lib/filter (lib/> orders-id 5000))
+ (lib/aggregate (lib/count))
+ (lib/aggregate (lib/cum-count))
+ (lib/breakout (lib/with-temporal-bucket orders-created-at :year))
+ (lib/breakout products-category)
+ (lib/limit 10)
+ (assoc-in [:middleware :format-rows?] false))]
+ (mt/with-native-query-testing-context query
+ ;; YEAR CATEGORY COUNT CUM-COUNT
+ (is (= [[#t "2016-01-01" "Doohickey" 131 131]
+ [#t "2016-01-01" "Gadget" 145 145]
+ [#t "2016-01-01" "Gizmo" 115 115]
+ [#t "2016-01-01" "Widget" 146 146]
+ [#t "2017-01-01" "Doohickey" 617 748] ; 131 + 617 = 748
+ [#t "2017-01-01" "Gadget" 690 835] ; 145 + 690 = 835
+ [#t "2017-01-01" "Gizmo" 616 731]
+ [#t "2017-01-01" "Widget" 718 864]
+ [#t "2018-01-01" "Doohickey" 865 1613]
+ [#t "2018-01-01" "Gadget" 1109 1944]]
+ (mt/formatted-rows
+ [->local-date str int int]
+ (qp/process-query query)))))))))
diff --git a/test/metabase/query_processor_test/offset_test.clj b/test/metabase/query_processor_test/offset_test.clj
index 61bbc506d81..af80cdadbfb 100644
--- a/test/metabase/query_processor_test/offset_test.clj
+++ b/test/metabase/query_processor_test/offset_test.clj
@@ -108,10 +108,10 @@
orders-created-at (lib.metadata/field metadata-provider (mt/id :orders :created_at))
orders-total (lib.metadata/field metadata-provider (mt/id :orders :total))
query (-> (lib/query metadata-provider orders)
- ;; 1. year
- (lib/breakout (lib/with-temporal-bucket orders-created-at :year))
- ;; 2. month
+ ;; 1. month
(lib/breakout (lib/with-temporal-bucket orders-created-at :month))
+ ;; 2. year
+ (lib/breakout (lib/with-temporal-bucket orders-created-at :year))
;; 3. sum(total)
(lib/aggregate (lib/sum orders-total))
;; 4. monthly growth %
@@ -123,18 +123,18 @@
(assoc-in [:middleware :format-rows?] false))]
(mt/with-native-query-testing-context query
;; 1 2 3 4
- (is (= [[#t "2016-01-01" #t "2016-04-01" 52.76 nil]
- [#t "2016-01-01" #t "2016-05-01" 1265.73 2299.03]
- [#t "2016-01-01" #t "2016-06-01" 2072.92 63.77]
- [#t "2016-01-01" #t "2016-07-01" 3734.72 80.17]
- [#t "2016-01-01" #t "2016-08-01" 4960.65 32.83]
- [#t "2016-01-01" #t "2016-09-01" 5372.09 8.29]
- [#t "2016-01-01" #t "2016-10-01" 7702.93 43.39]
- [#t "2016-01-01" #t "2016-11-01" 7926.69 2.9]
- [#t "2016-01-01" #t "2016-12-01" 9068.45 14.4]
- [#t "2017-01-01" #t "2017-01-01" 11094.77 nil] ; <- should reset here because breakout 1 changed values
- [#t "2017-01-01" #t "2017-02-01" 11243.66 1.34]
- [#t "2017-01-01" #t "2017-03-01" 14115.68 25.54]]
+ (is (= [[#t "2016-04-01" #t "2016-01-01" 52.76 nil]
+ [#t "2016-05-01" #t "2016-01-01" 1265.73 2299.03]
+ [#t "2016-06-01" #t "2016-01-01" 2072.92 63.77]
+ [#t "2016-07-01" #t "2016-01-01" 3734.72 80.17]
+ [#t "2016-08-01" #t "2016-01-01" 4960.65 32.83]
+ [#t "2016-09-01" #t "2016-01-01" 5372.09 8.29]
+ [#t "2016-10-01" #t "2016-01-01" 7702.93 43.39]
+ [#t "2016-11-01" #t "2016-01-01" 7926.69 2.9]
+ [#t "2016-12-01" #t "2016-01-01" 9068.45 14.4]
+ [#t "2017-01-01" #t "2017-01-01" 11094.77 nil] ; <- should reset here because breakout 2 changed values
+ [#t "2017-02-01" #t "2017-01-01" 11243.66 1.34]
+ [#t "2017-03-01" #t "2017-01-01" 14115.68 25.54]]
(mt/formatted-rows [->local-date ->local-date 2.0 2.0]
(qp/process-query query))))))))
--
GitLab