From 1c4ea0b136de88336916fe6a3542598d931bb97a Mon Sep 17 00:00:00 2001 From: Chris Truter Date: Fri, 16 Aug 2024 13:53:04 +0200 Subject: [PATCH] Fix phantom fields for boolean literals (#84) --- src/macaw/collect.clj | 5 +++++ .../{acceptance_tests.clj => acceptance_test.clj} | 6 ++++-- test/macaw/core_test.clj | 2 ++ .../acceptance/compound__cte_deadscope.analysis.edn | 10 ++++++++++ .../resources/acceptance/compound__cte_deadscope.sql | 4 ++++ .../compound__cte_nonambiguous.analysis.edn | 12 ++++++++++++ .../acceptance/compound__cte_nonambiguous.sql | 4 ++++ .../acceptance/compound__cte_pun.analysis.edn | 1 + test/resources/acceptance/cycle__cte.analysis.edn | 1 + .../acceptance/literal__with_table.analysis.edn | 2 ++ test/resources/acceptance/literal__with_table.sql | 1 + .../acceptance/literal__without_table.analysis.edn | 2 ++ test/resources/acceptance/literal__without_table.sql | 1 + .../acceptance/shadow__subselect.analysis.edn | 1 + 14 files changed, 50 insertions(+), 2 deletions(-) rename test/macaw/{acceptance_tests.clj => acceptance_test.clj} (96%) create mode 100644 test/resources/acceptance/compound__cte_deadscope.analysis.edn create mode 100644 test/resources/acceptance/compound__cte_deadscope.sql create mode 100644 test/resources/acceptance/compound__cte_nonambiguous.analysis.edn create mode 100644 test/resources/acceptance/compound__cte_nonambiguous.sql create mode 100644 test/resources/acceptance/literal__with_table.analysis.edn create mode 100644 test/resources/acceptance/literal__with_table.sql create mode 100644 test/resources/acceptance/literal__without_table.analysis.edn create mode 100644 test/resources/acceptance/literal__without_table.sql diff --git a/src/macaw/collect.clj b/src/macaw/collect.clj index d130e2e..2f0e2c0 100644 --- a/src/macaw/collect.clj +++ b/src/macaw/collect.clj @@ -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] @@ -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?) diff --git a/test/macaw/acceptance_tests.clj b/test/macaw/acceptance_test.clj similarity index 96% rename from test/macaw/acceptance_tests.clj rename to test/macaw/acceptance_test.clj index 9b345ce..3035a89 100644 --- a/test/macaw/acceptance_tests.clj +++ b/test/macaw/acceptance_test.clj @@ -1,4 +1,4 @@ -(ns macaw.acceptance-tests +(ns macaw.acceptance-test (:require [clojure.java.io :as io] [clojure.string :as str] @@ -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) @@ -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)) diff --git a/test/macaw/core_test.clj b/test/macaw/core_test.clj index 2b75eb1..b968148 100644 --- a/test/macaw/core_test.clj +++ b/test/macaw/core_test.clj @@ -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) diff --git a/test/resources/acceptance/compound__cte_deadscope.analysis.edn b/test/resources/acceptance/compound__cte_deadscope.analysis.edn new file mode 100644 index 0000000..60cbce7 --- /dev/null +++ b/test/resources/acceptance/compound__cte_deadscope.analysis.edn @@ -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"}]}} diff --git a/test/resources/acceptance/compound__cte_deadscope.sql b/test/resources/acceptance/compound__cte_deadscope.sql new file mode 100644 index 0000000..ac00530 --- /dev/null +++ b/test/resources/acceptance/compound__cte_deadscope.sql @@ -0,0 +1,4 @@ +WITH cte AS ( + SELECT x FROM t1 +) +SELECT x, y FROM t2 diff --git a/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn b/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn new file mode 100644 index 0000000..74d2ceb --- /dev/null +++ b/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn @@ -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"}]}} diff --git a/test/resources/acceptance/compound__cte_nonambiguous.sql b/test/resources/acceptance/compound__cte_nonambiguous.sql new file mode 100644 index 0000000..a957088 --- /dev/null +++ b/test/resources/acceptance/compound__cte_nonambiguous.sql @@ -0,0 +1,4 @@ +WITH cte AS ( + SELECT x FROM t1 +) +SELECT x, y FROM t2 JOIN cte diff --git a/test/resources/acceptance/compound__cte_pun.analysis.edn b/test/resources/acceptance/compound__cte_pun.analysis.edn index 789a8a4..1f1440d 100644 --- a/test/resources/acceptance/compound__cte_pun.analysis.edn +++ b/test/resources/acceptance/compound__cte_pun.analysis.edn @@ -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"} diff --git a/test/resources/acceptance/cycle__cte.analysis.edn b/test/resources/acceptance/cycle__cte.analysis.edn index 0634c04..5aa957c 100644 --- a/test/resources/acceptance/cycle__cte.analysis.edn +++ b/test/resources/acceptance/cycle__cte.analysis.edn @@ -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 #{}}} diff --git a/test/resources/acceptance/literal__with_table.analysis.edn b/test/resources/acceptance/literal__with_table.analysis.edn new file mode 100644 index 0000000..ed11ed0 --- /dev/null +++ b/test/resources/acceptance/literal__with_table.analysis.edn @@ -0,0 +1,2 @@ +{:tables [{:table "t"}] + :source-columns [{:table "t", :column "x"}]} diff --git a/test/resources/acceptance/literal__with_table.sql b/test/resources/acceptance/literal__with_table.sql new file mode 100644 index 0000000..40e34fc --- /dev/null +++ b/test/resources/acceptance/literal__with_table.sql @@ -0,0 +1 @@ +SELECT FALSE, 'str', 1, x FROM t diff --git a/test/resources/acceptance/literal__without_table.analysis.edn b/test/resources/acceptance/literal__without_table.analysis.edn new file mode 100644 index 0000000..973fef9 --- /dev/null +++ b/test/resources/acceptance/literal__without_table.analysis.edn @@ -0,0 +1,2 @@ +{:tables [] + :source-columns []} diff --git a/test/resources/acceptance/literal__without_table.sql b/test/resources/acceptance/literal__without_table.sql new file mode 100644 index 0000000..497a1b9 --- /dev/null +++ b/test/resources/acceptance/literal__without_table.sql @@ -0,0 +1 @@ +SELECT FALSE, 'str', 1 diff --git a/test/resources/acceptance/shadow__subselect.analysis.edn b/test/resources/acceptance/shadow__subselect.analysis.edn index acb01f3..02a0085 100644 --- a/test/resources/acceptance/shadow__subselect.analysis.edn +++ b/test/resources/acceptance/shadow__subselect.analysis.edn @@ -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"}