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: Replace connor package with rust implementation #2555

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

islamaliev
Copy link
Contributor

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

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

Specify the platform(s) on which this was tested:

  • MacOS

@islamaliev islamaliev added the refactor This issue specific to or requires *notable* refactoring of existing codebases and components label Apr 25, 2024
@islamaliev islamaliev requested a review from a team April 25, 2024 19:56
@islamaliev islamaliev self-assigned this Apr 25, 2024
@AndrewSisley
Copy link
Contributor

question: The CI is failing, are you wanting feedback before getting that to work?

@jsimnz
Copy link
Member

jsimnz commented Apr 29, 2024

Only done an quick look, nothing immediate code wise. But can you create a follow-up issue to change the null behavior for Numeric comparison on the Go connor implementation too. So that it matches the new behavior defined by the udpated tests in this PR?

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

libs/.gitkeep Outdated Show resolved Hide resolved
@islamaliev
Copy link
Contributor Author

Only done an quick look, nothing immediate code wise. But can you create a follow-up issue to change the null behavior for Numeric comparison on the Go connor implementation too. So that it matches the new behavior defined by the udpated tests in this PR?

I updated Go behaviour to match new expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor This issue specific to or requires *notable* refactoring of existing codebases and components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert a Go package into Rust
4 participants