accept table-qualified columns for renaming #33
Conversation
piranha
force-pushed
the
fully-qualified-renames
branch
from
9abcaf9
to
01be694
Compare
test/macaw/core_test.clj
Outdated
| {:tables {"a" "aa"} | ||
| :columns {{:table "a" :column "x"} "xx" | ||
| "y" "yy"}} | ||
| "SELECT aa.xx, b.x, b.yy FROM aa, b;") |
There was a problem hiding this comment.
Would be nice to have a test that flexed the table-and-schema-are-optional behavior. I guess that'd be a query like
select SUM(public.orders.amount) AS s, MAX(orders.amount) AS max, MIN(amount) AS min FROM public.orders
|
How about limiting the API to only work with Maybe it doesn't help simplify the most complex parts, but it's one less thing to test. e.g. we wouldn't need this |
|
I'm also still on the fence about schemas. You mostly don't have schemas when working with sql queries, but sometimes you do... So I'm on the verge between "whatever" and "try hard to match rename without schema to column with schemas". |
Co-Authored-By: Chris Truter <chris@metabase.com>
piranha
force-pushed
the
fully-qualified-renames
branch
from
cae74cf
to
5e7fe7b
Compare
| [macaw.core :as m] | ||
| [macaw.walk :as mw]) | ||
| (:import | ||
| (net.sf.jsqlparser.schema Table))) | ||
|
|
||
| (set! *warn-on-reflection* true) | ||
|
|
||
| (defn- normalize-ws [s] |
There was a problem hiding this comment.
Maybe we want to just go back to the test helper now that the tests are passing?
I prefer having something like this anyway to toggle debugging:
(defn- debug-test-replacement [before replacements _]
(m/replace-names before replacements))Or maybe a bit fancier:
(defmacro test-replacement [before replacement after]
`(let [result# (m/replace-names ~before ~replacements)]
(when *debug-replace* #p result#)
(is (= ~after result#)))
(binding [*debug-replace* true]
(test-replacement X Y Z))There was a problem hiding this comment.
I honestly prefer having calls to a tested function in place, since then you can execute it directly and see the result for yourself. So it's kind of unergonomic to have any wrappers...
There was a problem hiding this comment.
OK, so should we just totally scrap test-replacement?
There was a problem hiding this comment.
I'd remove it honestly, it's less intrusive then. But that's up to you, I've merged the pr already. 馃榿
Sooo after some pondering and experiments I extracted the whole data gathering facility because how am I going to get a table name in other way? 馃榿
Not to say this is not a best PR ever - I'm absolutely not sure what to do with schemas? Do we want specified schemas for tables too? I struggle to answer those questions, do you have any thoughts on what would be a more usable API?
Resolves #32