Support format preserving SQL transformations #10
Conversation
| /** | ||
| * Walk the given `expression`, invoking the callbacks for side effects as appropriate. | ||
| */ | ||
| public Expression walk(Expression expression) { |
There was a problem hiding this comment.
We don't strictly need this overloading, as we will still have this expression in scope in our calling Clojure, but this allows us to hide the implementation detail that the AST is mutable - potentially future proofing against a functional implementation.
I also think it's nice to semantically differentiate the cases and avoid leaking the null sentinel value.
src/macaw/walk.clj
Outdated
| (defn walk-query | ||
| "Walk over the query's AST, using the callbacks for their side-effects, for example to mutate the AST itself." | ||
| [parsed-query callbacks] | ||
| (.walk (AstWalker. (update-vals callbacks preserve) ::ignored) parsed-query)) |
There was a problem hiding this comment.
Using this sentinel based on a recent reading of the "nil should be the absence of only a few values" tip from the Naming section of Elements of Clojure. Not sure if the advice really applies here. The value should never leak in any case, and we're unlikely to break that.
| "select *, boink | ||
| , yoink as oink | ||
| from /* /* lore */ | ||
| core_user, | ||
| bore_user, /* more */ snore_user ;" |
There was a problem hiding this comment.
It's not fun writing or reading these multi-line strings here, it could be nice to break this corpus out into text and/or EDN test resource files.
| @@ -1,6 +1,6 @@ | |||
| (ns macaw.core-test | |||
| (:require | |||
| [clojure.test :refer :all] | |||
| [clojure.test :refer [deftest testing is]] | |||
There was a problem hiding this comment.
Leaking some Cursive linting
| (defn replace-names | ||
| "Given a SQL query, apply the given table and column renames." | ||
| [sql renames] | ||
| (rewrite/replace-names sql (parsed-query sql) renames)) |
There was a problem hiding this comment.
I fumbled a bit with how to structure the namespaces, and this smells quite a bit. If we adopt a Potemkin style facade-only namespace that would solve the circular dependency we're using this proxy to solve. I was a bit loathe to just add the (controversial to some) dependency yet though.
There was a problem hiding this comment.
Another alternative is to stick with a mono namespace for now.
| Table) | ||
| (net.sf.jsqlparser.statement | ||
| Statement))) | ||
| (net.sf.jsqlparser.parser CCJSqlParserUtil) |
There was a problem hiding this comment.
Whitespace wars, I find this a easier to scan personally unless the class list gets very long.
There was a problem hiding this comment.
If it passes Kondo I don't care :)
| (defn- update-query | ||
| "Emit a SQL string for an updated AST, preserving the comments and whitespace from the original SQL." | ||
| [updated-ast sql] | ||
| ;; work around ast visitor processing (inexplicably and incorrectly) duplicating expressions visits |
There was a problem hiding this comment.
Wanted to check if you know what's going on here before I file a bug. I hope it's not something grandfathered in through the TableNamesFinder.
crisptrutski
force-pushed
the
whitespace-preserving
branch
2 times, most recently
from
8e1194b to
8ed40e1
Compare
crisptrutski
force-pushed
the
enum-map
branch
from
fd602ae to
e371958
Compare
tsmacdonald
force-pushed
the
whitespace-preserving
branch
from
8ed40e1 to
ad7f7d7
Compare
tsmacdonald
force-pushed
the
enum-map
branch
from
e371958 to
ab981fe
Compare
tsmacdonald
force-pushed
the
whitespace-preserving
branch
from
ad7f7d7 to
dc933b0
Compare
crisptrutski
force-pushed
the
whitespace-preserving
branch
from
dc933b0 to
fa973cb
Compare
crisptrutski
force-pushed
the
whitespace-preserving
branch
from
fa973cb to
ac7522f
Compare
This mechanism seems to have some legs, but could breakdown if we do structural transformations.
The implementation can definitely be optimized and very likely simplified as well.