-
Notifications
You must be signed in to change notification settings - Fork 563
feat(tdigest): add TDIGEST.ADD command #2803
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
Conversation
91cb30f
to
d0b6d88
Compare
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.
General LGTM
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.
Hi @wyxxxcat ,
Thanks very much for your contribution! 😊
Generally LGTM, left some comments.
Best Regards,
Edward
d0b6d88
to
2a87b80
Compare
Hi @wyxxxcat , You could enable your fork repo's Github Action and it will trigger ci pipeline inside your own repo without the upstream repo's permission. It may accelerate your development and review process. 😁 Best Regards, |
499467d
to
c148fae
Compare
c148fae
to
f944bcb
Compare
|
Will merge if no negative comments tomorrow |
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.
Hi @wyxxxcat ,
Thanks very much for your effort! 😊
I left comment for error message compalibility with redis. I think it's not mandatory for tdigest command and current messages are reasonable, but will be better to keep compalibility.
Other parts are LGTM.
Best Regards,
Edward
Let's merge it first. Improvement can happen in further PRs. |
Issue
close: #2792
Proposed Changes
Comment
...