Port StatementRewrite::rewrite_simple_prepared to the new parser#1158
Port StatementRewrite::rewrite_simple_prepared to the new parser#1158sgrif wants to merge 1 commit into
StatementRewrite::rewrite_simple_prepared to the new parser#1158Conversation
I'm trying something new with this function, I'm intentionally using a silly form of cfg'ing the old code out to force those lines to have an indentation change, as this causes the diff to interpret the new code as changed lines and the old code as an addition. This will result in less churn in the long run (since the old code is getting deleted), and gives way nicer diffs for review
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| self.stmt.stmts = pg_query::parse(pg_raw_parse::deparse(node.as_ref())?.as_str())? | ||
| .protobuf | ||
| .stmts; |
There was a problem hiding this comment.
can you help understand this? do we need to update stmt just to push the modified version to self and provide the compatibility with old parser types? and for old parser it's not required because we do the in-place mutation of stmt?
There was a problem hiding this comment.
Correct. I'm parsing these functions in relatively small pieces, so some of the other functions called here modify or read from self.stmt instead of node. So any time a function modifies self.stmt we deparse and reparse into node, and any time we modify node, we need to deparse and reparse into self.stmt. Once everything in StatementRewrite is ported, this will no longer be necessary and I will cfg out the stmt field.
| let global_name = parse.name().to_string(); | ||
| let statement = parse.query().to_string(); | ||
| stmt.name = global_name.clone(); | ||
| stmt.set_name(Some(mem.copy_string(&global_name))); |
There was a problem hiding this comment.
I'm sorry for asking probably a dumb question, but I'm still at the start of my journey to understand the new parser. But could the set_name be simplified to not juggle with explicit mem operation to copy the data? I mean could the set_name handle it's by itself in its internals? I think the mem token is not yet stored on NodeMut, but probably we can store it or get the proper mem context somehow?
There was a problem hiding this comment.
You never need to apologize for asking questions. Part of the reason for code review is to spread knowledge 😄
But could the set_name be simplified to not juggle with explicit mem operation to copy the data? I mean could the set_name handle it's by itself in its internals?
We do this in constructor functions, but not setters. This is because the node doesn't have a reference to the memory context.
I think the mem token is not yet stored on NodeMut, but probably we can store it or get the proper mem context somehow?
We intentionally avoid keeping anything on Node/NodeMut other than a single pointer or zero-sized types. We want the node structs to be layout compatible with the C versions, and we want Node/NodeMut/&SomeNode/SomeNodeMut to all be layout identical to a *mut raw::Node, so that the conversion cost is nothing more than a pointer cast. (There's some caveats with Node/NodeMut that I'm happy to go into if you'd like but aren't relevant to this question)
There was a problem hiding this comment.
Shorter version of the answer: I don't see having to write mem.copy_string(s) as any different than having to write s.clone(). The only difference here is that we're using PG's allocator instead of the global one.
2 participants
I'm trying something new with this function, I'm intentionally using a silly form of cfg'ing the old code out to force those lines to have an indentation change, as this causes the diff to interpret the new code as changed lines and the old code as an addition. This will result in less churn in the long run (since the old code is getting deleted), and gives way nicer diffs for review