Port StatementRewrite::sharding_key_update to the new parser#1136
Port StatementRewrite::sharding_key_update to the new parser#1136sgrif wants to merge 9 commits into
Conversation
|
Once again going to assume |
There was a problem hiding this comment.
Provisionally agreed, but this will need cleanup - we got some more features coming in here soon and they will conflict - our current implementation is pretty easy to mod, this one might not be, I'm not sure. N.B.: I only had one coffee so far so I may be hallucinating issues.
|
I'll for sure look at adding a transforming AST walk (which is different than mutating, it will return a new ast) It'll probably after the port is done unless some other part of the code desperately needs it. I'll revisit this once I have that Though I'm gonna be honest, despite how hacky it is I don't agree that it's obviously worse than what the old code was doing. The sheer number of tests it has for various edge cases that just aren't necessary here says a lot. |
|
I'm gonna spike on both transforming and (limited) mutating AST walks on Monday. The more I think about it the more I realize at least one of those two is inevitable. My main concern is ensuring that they can be done safely, I don't want to have to have every ast walk be in an unsafe block. It's possible I'm being overly cautious. Anyway the more I think about it the more I realize it doesn't make sense to merge this until I've fully explored that problem. |
More specifically, this ports over the glue code as well as the last
subroutine of that function, `create_stmts`. This should be the beefiest
function we have to port (dear god please let this be the beefiest one).
The new implementation diverges from the old in a couple of ways:
- We deparse things lazily instead of eagerly
- The old code was only deparsing eagerly due to the slowness of
deparsing. This should no longer be slow enough to justify that. If
needed we can memoize the result, but I'd rather have it be a
memoized method than something upstream code cares about
- We do not rewrite the bind parameters, instead making every query use
all of them
- The old code did a recursive mutable AST walk to look for any bind
parameters in the query, rewrite/compress the numbers to be a
contiguous list starting from 1, and then mapepd that to the
original. I *really* don't want to have a mutable AST walk for the
new parser, so instead I just shove a no-op CTE that uses all the
params in the queries and copy the original bind over.
- ShardingKeyUpdate holds the entire original UpdateStmt rather than
smaller pieces
- I actually tried having this hold onto references of the original
query, but ultimately this struct ends up in a field that can't is a
sibling of the original AST, so it can't hold a lifetime like that.
I probably will revist that at some point, but for now copying the
whole thing made more sense than copying each smaller piece into its
own allocation
The unit tests look a little funky now to allow the new code to pass.
What they're testing is technically a little bit weaker than before (it
just checks that the bind parameters are all in the right places, it
gives zero shits about the numbers). However, once the port is finished,
I'll likely delete all but one of those unit tests. The complex logic
they were testing simply no longer exists, and we really just need one
test to assert that the where clause gets stuck on all 3 subqueries, and
that all 3 queries use the same bind parameters as the original update
The CTE hack for the bind params really is funky, but I couldn't come up
with any less hacky way to do it. I would very much prefer to stick
`SELECT $1, $2, ...` somewhere instead, but that won't work. We can't
rely on the client having sent us OIDs for the bind parameters in their
`Parse` message, and any binds that appear in a top level select will
get inferred to `text`. Since we can't assume any random blob of bytes
is valid UTF-8, that would cause the query to fail. Putting the original
update with `false AND` in the where clause was the simplest way I could
think of to ensure the query uses every bind parameter, and keep the
same type inference that we had before.
Other than the caveats mentioned above, the actual logic is mostly a
line for line port. The syntax is certainly very different for
constructing these trees, but it's a clean mapping each way.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
We're not caching the prepared statement, we're always using text format anyway, there's just no reason to do anything other than put the exact nodes we got in for the output
2 participants
More specifically, this ports over the glue code as well as the last subroutine of that function,
create_stmts. This should be the beefiest function we have to port (dear god please let this be the beefiest one).The new implementation diverges from the old in a couple of ways:
The unit tests look a little funky now to allow the new code to pass. What they're testing is technically a little bit weaker than before (it just checks that the bind parameters are all in the right places, it gives zero shits about the numbers). However, once the port is finished, I'll likely delete all but one of those unit tests. The complex logic they were testing simply no longer exists, and we really just need one test to assert that the where clause gets stuck on all 3 subqueries, and that all 3 queries use the same bind parameters as the original update
The CTE hack for the bind params really is funky, but I couldn't come up with any less hacky way to do it. I would very much prefer to stick
SELECT $1, $2, ...somewhere instead, but that won't work. We can't rely on the client having sent us OIDs for the bind parameters in theirParsemessage, and any binds that appear in a top level select will get inferred totext. Since we can't assume any random blob of bytes is valid UTF-8, that would cause the query to fail. Putting the original update withfalse ANDin the where clause was the simplest way I could think of to ensure the query uses every bind parameter, and keep the same type inference that we had before.Other than the caveats mentioned above, the actual logic is mostly a line for line port. The syntax is certainly very different for constructing these trees, but it's a clean mapping each way.