Skip to content

Commit

Permalink
Fix phantom fields for boolean literals (#84)
Browse files Browse the repository at this point in the history
  • Loading branch information
crisptrutski authored Aug 16, 2024
1 parent 3b0d3ed commit 1c4ea0b
Show file tree
Hide file tree
Showing 14 changed files with 50 additions and 2 deletions.
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
12 changes: 12 additions & 0 deletions test/resources/acceptance/compound__cte_nonambiguous.analysis.edn
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"}]}}
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
: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
2 changes: 2 additions & 0 deletions test/resources/acceptance/literal__without_table.analysis.edn
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

0 comments on commit 1c4ea0b

Please sign in to comment.