From f97a046ee2703f998587c0ab629b431729d17c93 Mon Sep 17 00:00:00 2001 From: Chris Truter Date: Wed, 14 Aug 2024 10:05:48 +0200 Subject: [PATCH 1/4] Filter out boolean literal columns --- src/macaw/collect.clj | 5 +++++ .../{acceptance_tests.clj => acceptance_test.clj} | 6 ++++-- test/macaw/core_test.clj | 4 ++++ .../compound__cte_nonambiguous.analysis.edn | 11 +++++++++++ .../acceptance/compound__cte_nonambiguous.sql | 4 ++++ .../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 + 9 files changed, 34 insertions(+), 2 deletions(-) rename test/macaw/{acceptance_tests.clj => acceptance_test.clj} (96%) 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..b4a2b0f 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 #p 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..effc92a 100644 --- a/test/macaw/core_test.clj +++ b/test/macaw/core_test.clj @@ -620,6 +620,10 @@ from foo") (require 'clojure.tools.namespace.repl) (virgil/watch-and-recompile ["java"] :post-hook clojure.tools.namespace.repl/refresh-all) + (source-columns "SELECT false") + (source-columns "SELECT false, x FROM t") + (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_nonambiguous.analysis.edn b/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn new file mode 100644 index 0000000..c1ae7db --- /dev/null +++ b/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn @@ -0,0 +1,11 @@ +{:tables [{:table "t1"} {:table "t2"}] + :source-columns [{:table "t1", :column "x"} ;; from cte + {:table "t2", :column "y"}] ;; from outer select + + :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..ac00530 --- /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 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 From 518ce7f3a41be10a5cb2d475425c61aa40a21430 Mon Sep 17 00:00:00 2001 From: Chris Truter Date: Wed, 14 Aug 2024 10:08:29 +0200 Subject: [PATCH 2/4] Remove rich comments for fixed queries --- test/macaw/core_test.clj | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/macaw/core_test.clj b/test/macaw/core_test.clj index effc92a..b968148 100644 --- a/test/macaw/core_test.clj +++ b/test/macaw/core_test.clj @@ -620,8 +620,6 @@ from foo") (require 'clojure.tools.namespace.repl) (virgil/watch-and-recompile ["java"] :post-hook clojure.tools.namespace.repl/refresh-all) - (source-columns "SELECT false") - (source-columns "SELECT false, x FROM t") (source-columns "WITH cte AS (SELECT x FROM t1) SELECT x, y FROM t2") (anonymize-query "SELECT x FROM a") From 547cac55a23d23e4e747297fa8ca2453f48362f4 Mon Sep 17 00:00:00 2001 From: Chris Truter Date: Wed, 14 Aug 2024 11:02:11 +0200 Subject: [PATCH 3/4] oops, left behind a #p --- src/macaw/collect.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/macaw/collect.clj b/src/macaw/collect.clj index b4a2b0f..2f0e2c0 100644 --- a/src/macaw/collect.clj +++ b/src/macaw/collect.clj @@ -159,7 +159,7 @@ (defn- literal? [{:keys [column]}] ;; numbers and strings are already handled by JSQLParser - (#{"true" "false"} (str/lower-case #p column))) + (#{"true" "false"} (str/lower-case column))) (defn- remove-redundant-columns "Remove any unqualified references that would resolve to an alias or qualified reference" From 42a22bc1abe4470fc7cb59254f830ce061014e81 Mon Sep 17 00:00:00 2001 From: Chris Truter Date: Fri, 16 Aug 2024 13:39:23 +0200 Subject: [PATCH 4/4] Add links back to issues --- .../acceptance/compound__cte_deadscope.analysis.edn | 10 ++++++++++ test/resources/acceptance/compound__cte_deadscope.sql | 4 ++++ .../acceptance/compound__cte_nonambiguous.analysis.edn | 1 + .../acceptance/compound__cte_nonambiguous.sql | 2 +- .../acceptance/compound__cte_pun.analysis.edn | 1 + test/resources/acceptance/cycle__cte.analysis.edn | 1 + .../acceptance/shadow__subselect.analysis.edn | 1 + 7 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 test/resources/acceptance/compound__cte_deadscope.analysis.edn create mode 100644 test/resources/acceptance/compound__cte_deadscope.sql 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 index c1ae7db..74d2ceb 100644 --- a/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn +++ b/test/resources/acceptance/compound__cte_nonambiguous.analysis.edn @@ -2,6 +2,7 @@ :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 diff --git a/test/resources/acceptance/compound__cte_nonambiguous.sql b/test/resources/acceptance/compound__cte_nonambiguous.sql index ac00530..a957088 100644 --- a/test/resources/acceptance/compound__cte_nonambiguous.sql +++ b/test/resources/acceptance/compound__cte_nonambiguous.sql @@ -1,4 +1,4 @@ WITH cte AS ( SELECT x FROM t1 ) -SELECT x, y FROM t2 +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/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"}