Conversation
harshit-gangal
force-pushed
the
set-parser-update
branch
from
1e6b970 to
77441bb
Compare
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Harshit Gangal <harshit@planetscale.com>
Signed-off-by: Andres Taylor <andres@planetscale.com>
harshit-gangal
force-pushed
the
set-parser-update
branch
from
77441bb to
ff80083
Compare
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Looks good overall. Some nits.
I feel like the Rewriter framework is complex, mainly because of going down vs. coming up, but I assume it's necessary. We avoided the "coming up" for the expression rewriter by using the sub-Rewrite. Not sure if the same thing can be done for this case.
It will require us to chain the rewriters, which is actually an easier pattern to explain and read, and should scale better. I feel like you'll piling on more rewriters as things evolve.
|
|
||
| //IsSetStatement takes Statement and returns if the statement is set statement. | ||
| func IsSetStatement(stmt Statement) bool { | ||
| switch stmt.(type) { |
There was a problem hiding this comment.
_, ok := stmt.(*Set)
return ok
| exp, err := n.normalizeSetExpr(expr) | ||
| if err != nil { | ||
| n.err = err | ||
| return false |
There was a problem hiding this comment.
Have you tested this code path? I see a panic in rewriter.go if this returns false.
There was a problem hiding this comment.
Yes, it is tested in the unit tests. All cases where we expect errors pass this path. The magic is here: https://github.com/vitessio/vitess/blob/master/go/vt/sqlparser/rewriter_api.go#L40
I used up vs down a lot when doing semantic checking on the AST in another project - scope is something you do top-down (you push down the scope to the children), whereas typing has to be done bottom-up (you need the types of children to calculate type of the parent). When I found the rewrite util here https://github.com/golang/tools/blob/master/go/ast/astutil/rewrite.go I thought that it was a nice API and gave me all I wanted. We can definitely change it if you feel it's too complex - we don't really need that capability today. Maybe tomorrow, but I'm guessing. What I think we need today is:
Should we set aside some time and change the rewriter infrastructure? Since it's generated, it should be pretty easy to do. |
3 participants
No description provided.