Skip to content
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

refactor: separate expression tree nodes into a separate tree from ast #3126

Merged
merged 1 commit into from
Jul 26, 2019

Conversation

rodesai
Copy link
Contributor

@rodesai rodesai commented Jul 25, 2019

Description

This patch moves the nodes in our expression tree into a separate parse
tree. This is a first step towards moving the code that deals with
expressions into ksql-execution (see #3125),
so that the serialized execution plan nodes can refer to expressions (e.g. for a select).

Both AST and Expression node have a common parent Node class, which now
just contains a NodeLocation, and abstract hashcode/equals methods.

The Expression tree now has its own visitor interface, called
ExpressionVisitor. This patch also migrates ExpressionFormatter to
implement this interface.

Note that this is just the first of multiple steps to get these decoupled, with the goal
for this PR being to separate the two trees. I'm deferring the following to later PRs:
- Actually moving the expression classes into ksql-execution (this pollutes the diff).
- We have lots of AstVisitor implementations that only operate on expressions
(e.g. codegen, expression type manager, insert-into analyzer). Migrating these
to implement ExpressionVisitor is deferred to a later PR.
- The current patch has AstVisitor implement ExpressionVisitor. These should be
decoupled into separate visitors (and all made private inner classes (see below)),
and explicitly composed where needed.
- ExpressionVisitor is an interface, which means that all the visit methods are public,
but that's fine since the final implementation should always be a private inner class
so that the interface can be restricted appropriately for the given visitor use case.
Migrating AstVisitor (and cleaning up the weird inheritance hierarchy) to follow a
similar pattern is also deferred.

Testing done

This is purely a refactor, so no new tests have been added.

@rodesai rodesai requested a review from a team as a code owner July 25, 2019 09:15
This patch moves the nodes in our expression tree into a separate parse
tree. This is a first step towards moving the code that deals with
expressions into ksql-execution, so that execution plan nodes can refer
to expressions (e.g. for a select).

Both AST and Expression node have a common parent Node class, which now
just contains a NodeLocation, and abstract hashcode/equals methods.

The Expression tree now has its own visitor interface, called
ExpressionVisitor. This patch also migrates ExpressionFormatter to
implement this interface.
Copy link
Contributor

@agavra agavra left a 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 detailed description :) helped answer questions

@agavra agavra requested a review from a team July 25, 2019 20:05
@rodesai rodesai merged commit 529c1f5 into confluentinc:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants