Skip to content

Commit

Permalink
[QP] Correctly find joined fields in the previous stage (#40933)
Browse files Browse the repository at this point in the history
This logic was trying to match `[name join-alias]` pairs previously, but
that won't work since the later stage field doesn't have a `:join-alias`
on it. This falls back to matching on only the ID or name, but only if
it's unambiguous.

Fixes #40252.
  • Loading branch information
bshepherdson committed Apr 3, 2024
1 parent 7a4b2fb commit cb50645
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ const modelB = {
type: "model",
};

// TODO: unskip when 40252 is fixed
describe.skip("issue 40252", () => {
describe("issue 40252", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
Expand Down
7 changes: 6 additions & 1 deletion src/metabase/query_processor/util/add_alias_info.clj
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,12 @@
(when (string? field-name)
(when-let [column (m/find-first #(= (:name %) field-name) source-metadata)]
(let [signature (field-signature (:field_ref column))]
(m/find-first #(= (field-signature %) signature) field-exports))))))))
(or ;; First try to match with the join alias.
(m/find-first #(= (field-signature %) signature) field-exports)
;; Then just the names, but if the match is ambiguous, warn and return nil.
(let [matches (filter #(= (second %) field-name) field-exports)]
(when (= (count matches) 1)
(first matches)))))))))))

(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
Expand Down
66 changes: 66 additions & 0 deletions test/metabase/query_processor/util/add_alias_info_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
[clojure.walk :as walk]
[metabase.driver :as driver]
[metabase.driver.h2 :as h2]
[metabase.lib.core :as lib]
[metabase.lib.test-metadata :as meta]
[metabase.lib.test-util :as lib.tu]
[metabase.lib.test-util.macros :as lib.tu.macros]
[metabase.lib.test-util.metadata-providers.mock :as providers.mock]
[metabase.query-processor.middleware.fix-bad-references
:as fix-bad-refs]
[metabase.query-processor.preprocess :as qp.preprocess]
Expand Down Expand Up @@ -772,3 +774,67 @@
::add/source-alias "CREATED_AT"
::add/desired-alias "Products__CREATED_AT"}]
(#'add/matching-field-in-join-at-this-level source-query field-clause))))))

(defn- metadata-provider-with-two-models []
(let [result-metadata-for (fn [column-name]
{:display_name column-name
:field_ref [:field column-name {:base-type :type/Integer}]
:name column-name
:base_type :type/Integer
:effective_type :type/Integer
:semantic_type nil
:fingerprint
{:global {:distinct-count 1, :nil% 0}
:type #:type{:Number {:min 1, :q1 1, :q3 1, :max 1, :sd nil, :avg 1}}}})]
(lib/composed-metadata-provider
meta/metadata-provider
(providers.mock/mock-metadata-provider
{:cards [{:name "Model A"
:id 1
:database-id (meta/id)
:type :model
:dataset-query {:database (mt/id)
:type :native
:native {:template-tags {} :query "select 1 as a1, 2 as a2;"}}
:result-metadata [(result-metadata-for "A1")
(result-metadata-for "A2")]}
{:name "Model B"
:id 2
:database-id (meta/id)
:type :model
:dataset-query {:database (mt/id)
:type :native
:native {:template-tags {} :query "select 1 as b1, 2 as b2;"}}
:result-metadata [(result-metadata-for "B1")
(result-metadata-for "B2")]}
{:name "Joined"
:id 3
:database-id (meta/id)
:type :model
:dataset-query {:database (meta/id)
:type :query
:query {:joins
[{:fields :all,
:alias "Model B - A1",
:strategy :inner-join,
:condition
[:=
[:field "A1" {:base-type :type/Integer}]
[:field "B1" {:base-type :type/Integer, :join-alias "Model B - A1"}]],
:source-table "card__2"}],
:source-table "card__1"}}}]}))))

(deftest ^:parallel models-with-joins-and-renamed-columns-test
(testing "an MBQL model with an explicit join and customized field names generate correct SQL (#40252)"
(qp.store/with-metadata-provider (metadata-provider-with-two-models)
(is (=? {:query {:fields [[:field "A1" {::add/source-table ::add/source
::add/source-alias "A1"}]
[:field "A2" {::add/source-table ::add/source
::add/source-alias "A2"}]
[:field "B1" {::add/source-table ::add/source
::add/source-alias "Model B - A1__B1"}]
[:field "B2" {::add/source-table ::add/source
::add/source-alias "Model B - A1__B2"}]]}}
(add-alias-info {:type :query
:database (meta/id)
:query {:source-table "card__3"}}))))))

0 comments on commit cb50645

Please sign in to comment.