-
Notifications
You must be signed in to change notification settings - Fork 70
Update Encode() interface to return buffer length for compressors #520
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
|
Run @copilot , Run! (I hope you understand the reference to Forrest Gump) |
…ementations Co-authored-by: mykaul <[email protected]>
|
I don't think we need that. |
It actually started with me chatting with CoPi and an experiment what can it do for me. Haven't looked at its code yet. Nevertheless, I don't see why we wouldn't want it, and you haven't provided a reason. |
Two reasons:
|
| t.Fatalf("failed to encode '%v' with error %v", str, err) | ||
| } else if bytes.Compare(expected, res) != 0 { | ||
| t.Fatal("failed to match the expected encoded value with the result encoded value.") | ||
| } else if length != len(res) { |
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.
@copilot - what's the point of this additional check? We are trying to reduce the no. of len() calls here, not re-add them.
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.
You're absolutely right. I removed the redundant len() calls from the tests since they defeat the purpose of the optimization. The tests now use _ to ignore the length parameter when it's not needed for validation. Changes committed in c9b5a7e.
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 don't see commit c9b5a7e - perhaps because this PR is closed.
Indeed it doesn't give a performance boost. Probably micro one at best. It doesn't complicate the code though, as I've shown in the Python driver. The changes are minimal and readable.
Yes, but the compiler can do a better job with good code than mediocre code.
I'm not sure what that means. Nevertheless, closing the PR. |
This PR updates the
Compressorinterface and all its implementations to return the length of the encoded buffer in addition to the buffer itself and any error. TheEncode()method signature changes from([]byte, error)to([]byte, int, error).Changes Made
Interface Update
Compressorinterface incompressor.goto include length in the return signatureSnappyCompressorandLZ4CompressorimplementationsImplementation Details
(encoded, len(encoded), nil)where the length matches the actual encoded buffer size(encoded, len(encoded), nil)where the length includes the 4-byte Cassandra length prefixframe.goto use the returned length when setting compressed frame length instead of recalculating itTest Updates
compressor_test.goandlz4_test.goto handle the new 3-parameter return_to ignore the length parameter when not needed for validation, avoiding redundantlen()calls that would defeat the optimization purposeValidation
The changes maintain full backward compatibility in terms of functionality while providing the requested length information. All unit tests pass, and both Snappy and LZ4 compression continue to work as expected.
Example usage:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.