Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix phantom fields for boolean literals #84

Merged
merged 4 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/macaw/collect.clj
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,10 @@
(cond-> (merge a b)
cs-a (update-in [:component :instances] into cs-a))))

(defn- literal? [{:keys [column]}]
;; numbers and strings are already handled by JSQLParser
(#{"true" "false"} (str/lower-case column)))

(defn- remove-redundant-columns
"Remove any unqualified references that would resolve to an alias or qualified reference"
[alias? column-set]
Expand Down Expand Up @@ -217,6 +221,7 @@
strip-alias (fn [c] (dissoc c :alias))
source-columns (->> (map :component all-columns)
(remove-redundant-columns alias?)
(remove literal?)
(into #{}
(comp (remove (comp pseudo-table-names :table))
(remove :internal?)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
(ns macaw.acceptance-tests
(ns macaw.acceptance-test
(:require
[clojure.java.io :as io]
[clojure.string :as str]
Expand All @@ -25,7 +25,6 @@
(defn- fixture-rewritten [fixture]
(some-> fixture (ct/fixture->filename "acceptance" ".rewritten.sql") io/resource slurp))


(defn- get-component [cs k]
(case k
:source-columns (get cs k)
Expand Down Expand Up @@ -98,5 +97,8 @@
(ns-unmap *ns* sym)))

(test-fixture :compound/cte)
(test-fixture :compound/cte-nonambiguous)
(test-fixture :literal/with-table)
(test-fixture :literal/without-table)

(test-fixture :broken/filter-where))
2 changes: 2 additions & 0 deletions test/macaw/core_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,8 @@ from foo")
(require 'clojure.tools.namespace.repl)
(virgil/watch-and-recompile ["java"] :post-hook clojure.tools.namespace.repl/refresh-all)

(source-columns "WITH cte AS (SELECT x FROM t1) SELECT x, y FROM t2")

(anonymize-query "SELECT x FROM a")
(anonymize-fixture :snowflake)
(anonymize-fixture :snowflakelet)
Expand Down
10 changes: 10 additions & 0 deletions test/resources/acceptance/compound__cte_deadscope.analysis.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
;; If we decide to perform "dead scope" elimination, t1 would not be listed as a source either.
{:tables [{:table "t1"} {:table "t2"}]
:source-columns [{:table "t2", :column "x"} ;; from cte
{:table "t2", :column "y"}] ;; from outer select

;; See https://github.com/metabase/metabase/issues/42586
:overrides
;; We are not taking into account the t1 (via cte) is not in-scope in the top-level SELECT.
{:source-columns [{:column "x"}
{:column "y"}]}}
4 changes: 4 additions & 0 deletions test/resources/acceptance/compound__cte_deadscope.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
WITH cte AS (
SELECT x FROM t1
)
SELECT x, y FROM t2
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{:tables [{:table "t1"} {:table "t2"}]
:source-columns [{:table "t1", :column "x"} ;; from cte
{:table "t2", :column "y"}] ;; from outer select

;; See https://github.com/metabase/metabase/issues/42586
:overrides
;; We are not taking into account that x is introduced with only t1 in scope.
;; We are not taking into account that x must not be an ambiguous reference in
;; the top-level query.
{:source-columns [{:column "x"}
;; We are not excluding the CTE, whose outputs are known, as a source for y.
{:column "y"}]}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the result here is incorrect, because x is from both t1 and t2, since cte is not referenced anywhere:

testmetabase=# with cte as (select id from core_user limit 1) select id, name from report_card limit 2;
 id |              name              
----+--------------------------------
  2 | Orders with People
  1 | Aerodynamic Copper Knife trend
(2 rows)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I meant for cte to be in scope.

mysql > with cte as (select id from core_user limit 1)
        select id name from report_card join cte limit 2;

ERROR 1052 (23000): Column 'id' in field list is ambiguous

Since Macaw is garbage-in-garbage-out for invalid queries, I think the correct conclusion is that id come from core_user only.

As for the actual query I wrote here, I guess you're right that in the short term returning both qualifications makes the most sense. One could imagine it doing some "dead scope analysis" however and excluding both the table and fields for the unused CTE.

4 changes: 4 additions & 0 deletions test/resources/acceptance/compound__cte_nonambiguous.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
WITH cte AS (
SELECT x FROM t1
)
SELECT x, y FROM t2 JOIN cte
1 change: 1 addition & 0 deletions test/resources/acceptance/compound__cte_pun.analysis.edn
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
{:table "report_dashboardcard", :column "card_id"}
{:table "report_dashboardcard", :column "created_at"}] ;; from outer select

;; See https://github.com/metabase/metabase/issues/42586
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using links instead of just #1212312 to disambiguate whether it's tracked in metabase or macaw.

:overrides
;; TODO We are missing some fields and some table qualifiers.
{:source-columns [{:column "created_at"}
Expand Down
1 change: 1 addition & 0 deletions test/resources/acceptance/cycle__cte.analysis.edn
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
{:table "a" :column "y"}
{:table "a" :column "z"}}

;; See https://github.com/metabase/metabase/issues/42586
:overrides
;; TODO currently all the sources get cancelled out with the derived columns due to analysis having flat scope.
{:source-columns #{}}}
2 changes: 2 additions & 0 deletions test/resources/acceptance/literal__with_table.analysis.edn
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{:tables [{:table "t"}]
:source-columns [{:table "t", :column "x"}]}
1 change: 1 addition & 0 deletions test/resources/acceptance/literal__with_table.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SELECT FALSE, 'str', 1, x FROM t
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{:tables []
:source-columns []}
1 change: 1 addition & 0 deletions test/resources/acceptance/literal__without_table.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
SELECT FALSE, 'str', 1
1 change: 1 addition & 0 deletions test/resources/acceptance/shadow__subselect.analysis.edn
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
{:table "employees" :column "last_name"}
{:table "employees" :column "department_id"}}

;; See https://github.com/metabase/metabase/issues/42586
:overrides
{:source-columns #{{:table "departments" :column "id"}
{:table "departments" :column "name"}
Expand Down
Loading