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

feat(binder): egg binder for drop statement #711

Merged
merged 5 commits into from
Oct 16, 2022

Conversation

eliasyaoyc
Copy link
Contributor

Signed-off-by: Elias.Yao <siran0611@gmail.com>
Signed-off-by: Elias.Yao <siran0611@gmail.com>
Signed-off-by: Elias.Yao <siran0611@gmail.com>
@wangrunji0408 wangrunji0408 self-requested a review October 14, 2022 15:56
Copy link
Member

@wangrunji0408 wangrunji0408 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM. Thanks!

Signed-off-by: Elias.Yao <siran0611@gmail.com>
@wangrunji0408 wangrunji0408 enabled auto-merge (squash) October 15, 2022 04:56
@skyzh skyzh disabled auto-merge October 15, 2022 16:24
@skyzh
Copy link
Member

skyzh commented Oct 15, 2022

Indeed I'm wondering why we're commenting out the original drop and add a new BoundDrop between the expressions. Please hold this PR a little while and I will refactor tonight.

@wangrunji0408
Copy link
Member

wangrunji0408 commented Oct 15, 2022

Indeed I'm wondering why we're commenting out the original drop and add a new BoundDrop between the expressions. Please hold this PR a little while and I will refactor tonight.

I thought both ways are acceptable. Given that the drop statement is unlikely to be rewritten by rules, representing it as a single BoundDrop value can avoid parsing it back from the Expr in the executor. 🤔

Copy link
Member

@skyzh skyzh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Some futures plans in my mind:

  • Why insert / create / drop are part of pub enum Expr? 🤣 Seems a little bit weird but I'm thinking if it is possible to make it more clear.
  • I believe we only need to support Drop(Id) in RisingLight. We can remove cascade / remove_if_exist later.

@skyzh skyzh merged commit 1fd9226 into risinglightdb:main Oct 16, 2022
@wangrunji0408
Copy link
Member

Why insert / create / drop are part of pub enum Expr? 🤣 Seems a little bit weird but I'm thinking if it is possible to make it more clear.

Yeah, putting them all into the Expr is just for making a single AST representation in the planner, although these statements won't benefit much from the egg's features since there is no need to rewrite or analyze them.

@eliasyaoyc eliasyaoyc deleted the feat/new-binder-statement branch October 27, 2022 02:55
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.

3 participants