-
Notifications
You must be signed in to change notification settings - Fork 50
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: Replace connor package with rust implementation #2555
base: develop
Are you sure you want to change the base?
refactor: Replace connor package with rust implementation #2555
Conversation
question: The CI is failing, are you wanting feedback before getting that to work? |
Only done an quick look, nothing immediate code wise. But can you create a follow-up issue to change the |
connor/ge.go
Outdated
@@ -11,8 +11,7 @@ import ( | |||
// value is strictly larger than or equal to another. | |||
func ge(condition, data any) (bool, error) { | |||
if condition == nil { | |||
// Everything is greater than or equal to nil | |||
return true, nil | |||
return false, nil |
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.
question: What caused this change?
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.
We changed the behaviour. Previously a condition 4 > null, 4 >= null and others would evaluate to true. But we decided to change it. We also discussed (in Discord) that ideally we would want to return an error for comparisons like this.
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 dont think this change should be made in this PR. Besides the lack of transparency in the change log, and more complex PRs, only including it in this PR is also a good way of making sure that the ge
change doesnt get merged for a few months.
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.
The rust implementation already behaves this way. I don't think it's a good idea to keep different behaviours. Otherwise you won't be able to switch between them without breaking existing tests.
And the idea behind it was anyway to get rid of golang version once we have Rust substitute because we don't want to maintain 2 implementations in parallel.
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.
The rust implementation already behaves this way.
The rust implementation is not used. Change the Go one, then the Rust one will pair up with it.
I updated Go behaviour to match new expectations. |
264681c
to
4d953f6
Compare
Relevant issue(s)
Resolves #2508
Description
Replaces go implementation of connor package with rust implementation. All the input into connor package is serialized into JSON format understandable by rust implementation and passed across ABI boundary.
JSON was chosen just to keep things as simple as possible. We are not going keep the current state long (hopefully).
Note: We shouldn't merge this PR unless we are ready to allocate some human resources into transitioning DefraDB to Rust.
Tasks
Specify the platform(s) on which this was tested: