-
-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply bindvars to subqueries #871
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix
sql/plan/transform.go
Outdated
// TransformUpWithOpaque applies a transformation function to the given tree from the bottom up, including through | ||
// opaque nodes. This method is generally not safe to use for a transformation. Opaque nodes need to be considered in | ||
// isolation except for very specific exceptions. | ||
// TODO: a better way to do this might be to keep the WITH nodes around until the very end of anlysis, so that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this TODO, doesn't make sense in isolation
sql/plan/bindvar.go
Outdated
|
||
return TransformUpWithOpaque(n, func(node sql.Node) (sql.Node, error) { | ||
switch n := node.(type) { | ||
case *IndexedJoin: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems misplaced, left over from other PR?
sql/plan/bindvar.go
Outdated
} | ||
return e, nil | ||
return TransformExpressionsUp(node, fixBindings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a default label rather than outside the switch
Cherry pick from #795 to fix Subquery bindvars specficially.