Skip to content
Snippets Groups Projects
Unverified Commit 53b9dd69 authored by Ngoc Khuat's avatar Ngoc Khuat Committed by GitHub
Browse files

Do not merge metadata for aggregation fields (#24896) 

* the fix

* adding some tests

* alternative solution in which we don't merge metadata for aggregation fields at all

* wording for the docstring

* use partial= and remove smart quotes. Thanks Cam
parent 29812d24
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/query_processor/middleware/annotate.clj
+ 8
− 4
View file @ 53b9dd69
...@@ -517,10 +517,14 @@ ...@@ -517,10 +517,14 @@
(defn- flow-field-metadata (defn- flow-field-metadata
"Merge information about fields from `source-metadata` into the returned `cols`." "Merge information about fields from `source-metadata` into the returned `cols`."
[source-metadata cols dataset?] [source-metadata cols dataset?]
(let [index (fn [col] (or (:id col) (:name col ""))) (let [by-key (m/index-by (comp qp.util/field-ref->key :field_ref) source-metadata)]
index->metadata (m/index-by index source-metadata)] (for [{:keys [field_ref source] :as col} cols]
(for [col cols] ;; aggregation fields are not from the source-metadata and their field_ref
(if-let [source-metadata-for-field (-> col index index->metadata)] ;; are not unique for a nested query. So do not merge them otherwise the metadata will be messed up.
;; TODO: I think the best option here is to introduce a parent_field_ref so that
;; we could preserve metadata such as :sematic_type or :unit from the source field.
(if-let [source-metadata-for-field (and (not= :aggregation source)
(get by-key (qp.util/field-ref->key field_ref)))]
(merge-source-metadata-col source-metadata-for-field (merge-source-metadata-col source-metadata-for-field
(merge col (merge col
(when dataset? (when dataset?
......
src/metabase/query_processor/util.clj
+ 3
− 2
View file @ 53b9dd69
...@@ -128,7 +128,8 @@ ...@@ -128,7 +128,8 @@
ensure survives." ensure survives."
[fresh pre-existing] [fresh pre-existing]
(let [by-key (m/index-by (comp field-ref->key :field_ref) pre-existing)] (let [by-key (m/index-by (comp field-ref->key :field_ref) pre-existing)]
(for [{:keys [field_ref] :as col} fresh] (for [{:keys [field_ref source] :as col} fresh]
(if-let [existing (get by-key (field-ref->key field_ref))] (if-let [existing (and (not= :aggregation source)
(get by-key (field-ref->key field_ref)))]
(merge col (select-keys existing preserved-keys)) (merge col (select-keys existing preserved-keys))
col)))) col))))
test/metabase/query_processor/middleware/annotate_test.clj
+ 37
− 1
View file @ 53b9dd69
...@@ -633,7 +633,43 @@ ...@@ -633,7 +633,43 @@
{:name "ID_2", :id %categories.id, :field_ref &c.categories.id} {:name "ID_2", :id %categories.id, :field_ref &c.categories.id}
{:name "NAME_2", :id %categories.name, :field_ref &c.categories.name}]) {:name "NAME_2", :id %categories.name, :field_ref &c.categories.name}])
(map #(select-keys % [:name :id :field_ref]) (map #(select-keys % [:name :id :field_ref])
(:cols (add-column-info nested-query {})))))))))))) (:cols (add-column-info nested-query {})))))))))))
(testing "Aggregated question with source is an aggregated models should infer display_name correctly (#23248)"
(mt/dataset sample-dataset
(mt/with-temp* [Card [{card-id :id}
{:dataset true
:dataset_query
(mt/$ids :products
{:type :query
:database (mt/id)
:query {:source-table $$products
:aggregation
[[:aggregation-options
[:sum $price]
{:name "sum"}]
[:aggregation-options
[:max $rating]
{:name "max"}]]
:breakout $category
:order-by [[:asc $category]]}})}]]
(let [query (qp/preprocess
(mt/mbql-query nil
{:source-table (str "card__" card-id)
:aggregation [[:aggregation-options
[:sum
[:field
"sum"
{:base-type :type/Float}]]
{:name "sum"}]
[:aggregation-options
[:count]
{:name "count"}]]
:limit 1}))]
(is (= ["Sum of Sum of Price" "Count"]
(->> (add-column-info query {})
:cols
(map :display_name)))))))))
(deftest inception-test (deftest inception-test
(testing "Should return correct metadata for an 'inception-style' nesting of source > source > source with a join (#14745)" (testing "Should return correct metadata for an 'inception-style' nesting of source > source > source with a join (#14745)"
......
test/metabase/query_processor_test/nested_queries_test.clj
+ 42
− 38
View file @ 53b9dd69
...@@ -147,44 +147,48 @@ ...@@ -147,44 +147,48 @@
:breakout [$venue_id->venues.price $user_id] :breakout [$venue_id->venues.price $user_id]
:limit 5})))))))) :limit 5}))))))))
(deftest nested-with-aggregations-at-both-levels (deftest nested-with-aggregations-at-both-levels-test
(testing "Aggregations in both nested and outer query have correct metadata (#19403)" (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
(mt/test-drivers (mt/normal-drivers-with-feature :nested-queries) (mt/dataset sample-dataset
(mt/dataset sample-dataset (doseq [dataset? [true false]]
(mt/with-temp* [Card [{card-id :id} (testing (format "Aggregations in both nested and outer query for %s have correct metadata (#19403) and (#23248)"
{:dataset_query (if dataset? "questions" "models"))
(mt/$ids :products (mt/with-temp* [Card [{card-id :id :as card}
{:type :query {:dataset dataset?
:database (mt/id) :dataset_query
:query {:source-table $$products (mt/$ids :products
:aggregation {:type :query
[[:aggregation-options :database (mt/id)
[:sum $price] :query {:source-table $$products
{:name "sum"}] :aggregation
[:aggregation-options [[:aggregation-options
[:max $rating] [:sum $price]
{:name "max"}]] {:name "sum"}]
:breakout $category [:aggregation-options
:order-by [[:asc $category]]}})}]] [:max $rating]
(is (= {:cols [{:name "count" :display_name "Count"} {:name "max"}]]
{:name "avg" :display_name "Average of Sum of Price"}] :breakout $category
:rows [[4 2787]]} :order-by [[:asc $category]]}})}]]
(-> (mt/format-rows-by [int int] (is (partial= {:data {:cols [{:name "sum" :display_name "Sum of Sum of Price"}
(qp/process-query {:type :query {:name "count" :display_name "Count"}]
:database (mt/id) :rows [[11149 4]]}}
:query {:source-table (str "card__" card-id) (mt/format-rows-by [int int]
:aggregation [[:aggregation-options (qp/process-query (merge {:type :query
[:count] :database (mt/id)
{:name "count"}] :query {:source-table (str "card__" card-id)
[:aggregation-options :aggregation [[:aggregation-options
[:avg [:sum
[:field [:field
"sum" "sum"
{:base-type :type/Float}]] {:base-type :type/Float}]]
{:name "avg"}]]}})) {:name "sum"}]
:data [:aggregation-options
(select-keys [:cols :rows]) [:count]
(update :cols #(map (fn [c] (select-keys c [:name :display_name])) %)))))))))) {:name "count"}]]}}
(when dataset?
{:info {:metadata/dataset-metadata (:result_metadata card)}}))))))))))))
(deftest sql-source-query-breakout-aggregation-test (deftest sql-source-query-breakout-aggregation-test
(mt/test-drivers (mt/normal-drivers-with-feature :nested-queries) (mt/test-drivers (mt/normal-drivers-with-feature :nested-queries)
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment