From 6858dcefac340054a069cc49673f75d237816e03 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Fri, 15 Jul 2022 22:26:27 +0300
Subject: [PATCH] Retain source alias for aggregates (#23771)
---
.../query_processor_test.clj | 19 ++++++
.../query_processor/util/add_alias_info.clj | 19 +++---
.../util/add_alias_info_test.clj | 62 ++++++++++++++++++-
3 files changed, 92 insertions(+), 8 deletions(-)
diff --git a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
index e0718acd5c1..9bbc6b6e47d 100644
--- a/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
+++ b/modules/drivers/bigquery-cloud-sdk/test/metabase/driver/bigquery_cloud_sdk/query_processor_test.clj
@@ -854,3 +854,22 @@
(let [t [:absolute-datetime #t "2022-04-22T18:27:00-08:00"]
hsql-form (sql.qp/add-interval-honeysql-form :bigquery-cloud-sdk t 3 :month)]
(sql.qp/format-honeysql :bigquery-cloud-sdk hsql-form))))))
+
+(deftest custom-expression-with-space-in-having
+ (mt/test-driver :bigquery-cloud-sdk
+ (mt/dataset avian-singles
+ (testing "Custom expressions with spaces are matched properly (#22310)"
+ (let [name-with-spaces "sum id diff"
+ sql-query (-> (mt/mbql-query messages
+ {:filter [:> [:field name-with-spaces {:base-type :type/Float}] 5]
+ :source-query {:source-table $$messages
+ :aggregation [[:aggregation-options
+ [:sum [:- $sender_id $receiver_id]]
+ {:name name-with-spaces
+ :display-name name-with-spaces}]]
+ :breakout [$text]}
+ :limit 1})
+ qp/compile
+ :query)]
+ (is (not (str/includes? sql-query name-with-spaces))
+ (format "Query `%s' should not contain `%s'" sql-query name-with-spaces)))))))
diff --git a/src/metabase/query_processor/util/add_alias_info.clj b/src/metabase/query_processor/util/add_alias_info.clj
index 95a623c0d6a..d1beba4b9c3 100644
--- a/src/metabase/query_processor/util/add_alias_info.clj
+++ b/src/metabase/query_processor/util/add_alias_info.clj
@@ -186,7 +186,7 @@
(defn- exports [query]
(into #{} (mbql.u/match (dissoc query :source-query :source-metadata :joins)
- [(_ :guard #{:field :expression :aggregation}) _ (_ :guard (every-pred map? ::position))])))
+ [(_ :guard #{:field :expression :aggregation-options}) _ (_ :guard (every-pred map? ::position))])))
(defn- join-with-alias [{:keys [joins]} join-alias]
(some (fn [join]
@@ -216,12 +216,16 @@
(when-let [field-name (let [[_ id-or-name] field-clause]
(when (string? id-or-name)
id-or-name))]
- (when-let [expression-exports (not-empty (filter (partial mbql.u/is-clause? :expression) all-exports))]
- (some
- (fn [[_ expression-name :as expression-clause]]
- (when (= expression-name field-name)
- expression-clause))
- expression-exports))))))
+ (or (some
+ (fn [[_ expression-name :as expression-clause]]
+ (when (= expression-name field-name)
+ expression-clause))
+ (filter (partial mbql.u/is-clause? :expression) all-exports))
+ (some
+ (fn [[_ _ opts :as aggregation-options-clause]]
+ (when (= (::source-alias opts) field-name)
+ aggregation-options-clause))
+ (filter (partial mbql.u/is-clause? :aggregation-options) all-exports)))))))
(defn- matching-field-in-join-at-this-level
"If `field-clause` is the result of a join *at this level* with a `:source-query`, return the 'source' `:field` clause
@@ -347,6 +351,7 @@
unique-alias (unique-alias-fn position original-ag-name)]
[:aggregation-options wrapped-ag-clause (assoc opts
:name unique-alias
+ ::source-alias original-ag-name
::position position
::desired-alias unique-alias)]))
diff --git a/test/metabase/query_processor/util/add_alias_info_test.clj b/test/metabase/query_processor/util/add_alias_info_test.clj
index 335a5c82e85..21f65346ff9 100644
--- a/test/metabase/query_processor/util/add_alias_info_test.clj
+++ b/test/metabase/query_processor/util/add_alias_info_test.clj
@@ -187,6 +187,7 @@
{:aggregation [[:aggregation-options
[:count]
{:name "count"
+ ::add/source-alias "count"
::add/desired-alias "count"
::add/position 0}]]
:filter [:!=
@@ -211,16 +212,19 @@
:aggregation [[:aggregation-options
[:count]
{:name "count"
+ ::add/source-alias "count"
::add/desired-alias "count"
::add/position 0}]
[:aggregation-options
[:count]
{:name "count_2"
+ ::add/source-alias "count_2"
::add/desired-alias "count_2"
::add/position 1}]
[:aggregation-options
[:count]
{:name "count_3"
+ ::add/source-alias "count_3"
::add/desired-alias "count_3"
::add/position 2}]]}
:fields [[:field "count" {:base-type :type/BigInteger
@@ -305,6 +309,7 @@
[:avg [:field %reviews.rating {::add/source-table $$reviews
::add/source-alias "RATING"}]]
{:name "avg"
+ ::add/source-alias "avg"
::add/desired-alias "avg"
::add/position 1}]]
:breakout [[:field %products.category {:join-alias "P2"
@@ -370,9 +375,12 @@
(driver/register! ::custom-escape :parent :h2)
+(defn- prefix-alias [alias]
+ (str "COOL." alias))
+
(defmethod driver/escape-alias ::custom-escape
[_driver field-alias]
- (str "COOL." field-alias))
+ (prefix-alias field-alias))
(deftest custom-escape-alias-test
(let [db (mt/db)]
@@ -399,6 +407,7 @@
::add/position 0}]]
{:aggregation [[:aggregation-options [:count] {:name "COOL.count"
::add/position 1
+ ::add/source-alias "count"
::add/desired-alias "COOL.count"}]]
:breakout [double-price]
:order-by [[:asc double-price]]})))
@@ -413,6 +422,55 @@
add-alias-info
:query)))))))
+(deftest custom-escape-alias-filtering-aggregation-test
+ (let [db (mt/db)]
+ (driver/with-driver ::custom-escape
+ (mt/with-db db
+ (is (query= (mt/$ids venues
+ (let [price [:field %price {::add/source-table $$venues
+ ::add/source-alias "PRICE"
+ ::add/desired-alias "COOL.PRICE"
+ ::add/position 0}]
+ outer-price (-> price
+ (assoc-in [2 ::add/source-table] ::add/source)
+ (update-in [2 ::add/source-alias] prefix-alias)
+ (update-in [2 ::add/desired-alias] prefix-alias))
+ count-opts {:name "COOL.strange count"
+ ::add/source-alias "strange count"
+ ::add/desired-alias "COOL.strange count"
+ ::add/position 1}
+ outer-count-opts (-> count-opts
+ (dissoc :name)
+ (assoc :base-type :type/BigInteger
+ ::add/source-table ::add/source)
+ (update ::add/source-alias prefix-alias)
+ (update ::add/desired-alias prefix-alias))]
+ {:source-query
+ {:source-table $$venues
+ :breakout [price]
+ :aggregation [[:aggregation-options [:count] count-opts]]
+ :order-by [[:asc price]]}
+ :fields [outer-price
+ [:field
+ "strange count"
+ outer-count-opts]]
+ :filter [:<
+ [:field
+ "strange count"
+ outer-count-opts]
+ [:value 10 {:base_type :type/BigInteger}]]
+ :limit 1}))
+ (-> (mt/mbql-query venues
+ {:source-query {:source-table $$venues
+ :aggregation [[:aggregation-options
+ [:count]
+ {:name "strange count"}]]
+ :breakout [$price]}
+ :filter [:< [:field "strange count" {:base-type :type/BigInteger}] 10]
+ :limit 1})
+ add-alias-info
+ :query)))))))
+
(driver/register! ::custom-escape-spaces-to-underscores :parent :h2)
(defmethod driver/escape-alias ::custom-escape-spaces-to-underscores
@@ -555,6 +613,7 @@
::add/source-alias "USER_ID"}]]
{:name "sum"
::add/position 0
+ ::add/source-alias "sum"
::add/desired-alias "sum"}]]
:order-by [[:asc [:aggregation 0 {::add/desired-alias "sum"
::add/position 0}]]]})
@@ -569,6 +628,7 @@
:breakout [[:expression "count" {::add/desired-alias "count"
::add/position 0}]]
:aggregation [[:aggregation-options [:count] {:name "count_2"
+ ::add/source-alias "count"
::add/desired-alias "count_2"
::add/position 1}]]
:order-by [[:asc [:expression "count" {::add/desired-alias "count"
--
GitLab