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.

Sign up for GitHub

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

Jump to bottom

Small refactoring for expression visitor impl class #5435

Merged
merged 2 commits into from
Mar 24, 2023
Merged

Small refactoring for expression visitor impl class #5435

merged 2 commits into from
Mar 24, 2023

Conversation

czpmango
Copy link
Contributor

@czpmango czpmango commented Mar 24, 2023 •
edited
Loading

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

How do you solve it?

  • Move ExprVisitorImpl to common
  • Delete storage/ExprVisitorBase

Special notes for your reviewer, ex. impact of this fix, design document, etc:

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.）
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

@czpmango
move ExprVisitorImpl to common directory and delete storage/ExprVisit…
263e333 
…orBase

small remove

Delete ExprVisitorBase

small remove
@czpmango czpmango added ready-for-testing PR: ready for the CI test ready for review labels Mar 24, 2023
@Shylock-Hg
Copy link
Contributor

Why change this?

@czpmango
Copy link
Contributor Author

czpmango commented Mar 24, 2023 •
edited
Loading

Why change this?

Remove some LOG(FATAL) in ExprVisitorBase.cpp, there were some bugs that crashed the storaged service related to this code.
Besides, it is also reasonable to move the expression visitor implementation class to the common directory, IMHO.

@Sophie-Xie Sophie-Xie requested a review from critical27 March 24, 2023 04:36
critical27
critical27 approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job

@critical27 critical27 merged commit 2f5a57d into vesoft-inc:master Mar 24, 2023
@czpmango czpmango changed the title Small Refactoring for expression visitor impl class Small refactoring for expression visitor impl class Mar 24, 2023
@wey-gu wey-gu mentioned this pull request Mar 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shylock-Hg Shylock-Hg Shylock-Hg approved these changes

@cangfengzhs cangfengzhs cangfengzhs approved these changes

@critical27 critical27 critical27 approved these changes

Assignees
No one assigned
Labels
auto-sync ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@czpmango @Shylock-Hg @critical27 @cangfengzhs @Sophie-Xie