This repository has been archived by the owner on Jan 28, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 109
sql: implement new API for node transformation #773
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
juanjux
reviewed
Jun 27, 2019
juanjux
reviewed
Jun 27, 2019
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.
I solemnly declare here that I read everything.
juanjux
approved these changes
Jun 27, 2019
kuba--
reviewed
Jun 27, 2019
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.
For me personally all transformations are very fragile (if sth. mysterious happens, it's most likely in some transformation).
This PR is huge, but for the first insight, it reduces a little bit number of possible mistakes (even shadowed nodes).
ajnavarro
approved these changes
Jun 27, 2019
@erizocosmico could you rebase please? |
erizocosmico
force-pushed
the
feature/new-transforms
branch
from
July 3, 2019 14:54
2c7cd89
to
5fbc7f1
Compare
Done |
Instead of having TransformUp and TransformExpressionsUp in each node, which was really error prone, now we have a different API. Each node will have a WithChildren method that will receive the children from which a new node of the same type will be created. These nodes must come in the same number and order as the ones returned by the Children method. Expressioner nodes will also have WithExpressions method, which is the same, except it will create a new node with its expressions changed, instead of the children nodes. The plan package will expose 3 new helpers: - TransformUp: which transforms a node from the bottom-up. - TransformExpressionsUp: which transforms expressions of a node from the bottom up. - TransformExpressions: which transforms the expressions of just the given node. Just like with nodes, expressions will also have a new WithChildren method that does the exact same thing as it does in the nodes. The expression package will expose a new helper: - TransformUp: which transforms an expression from the bottom up. Caveats and limitations: One thing that may seem odd is the limitation that WithChildren and WithExpressions must receive the children in the exact same order and number as they were returned from Expressions or Children. This is because without this limitation there is no way to build certain nodes. If we force this limitation on one, it would feel odd not to have it elsewhere. For example, take Case expression into account. It may or may not have Expr, it has a list of branches (each having 2 expressions) and it may or may not have an Else expression. If WithChildren receives N children, how do we know where to put all these expressions? This limitation allows us to build the node beacause we know the shape of children must match the current shape. Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
erizocosmico
force-pushed
the
feature/new-transforms
branch
from
July 3, 2019 14:59
5fbc7f1
to
128924a
Compare
Updated so that resolve_having can take advantage of the new |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #772
Instead of having TransformUp and TransformExpressionsUp in each
node, which was really error prone, now we have a different API.
Each node will have a WithChildren method that will receive
the children from which a new node of the same type will be created.
These nodes must come in the same number and order as the ones
returned by the Children method.
Expressioner nodes will also have WithExpressions method, which is
the same, except it will create a new node with its expressions
changed, instead of the children nodes.
The plan package will expose 3 new helpers:
the bottom up.
given node.
Just like with nodes, expressions will also have a new WithChildren
method that does the exact same thing as it does in the nodes.
The expression package will expose a new helper:
The sql package now has one interface more:
OpaqueNode
. An opaque node is a node that acts as a black box and their children will not be transformed using the aforementioned helpers. SubqueryAlias node, for example, requires being opaque.Caveats and limitations:
One thing that may seem odd is the limitation that WithChildren and
WithExpressions must receive the children in the exact same order and
number as they were returned from Expressions or Children.
This is because without this limitation there is no way to build
certain nodes. If we force this limitation on one, it would feel odd
not to have it elsewhere.
For example, take Case expression into account. It may or may not have
Expr, it has a list of branches (each having 2 expressions) and it
may or may not have an Else expression. If WithChildren receives N
children, how do we know where to put all these expressions?
This limitation allows us to build the node beacause we know the shape
of children must match the current shape.