Skip to content

Conversation

mellis13
Copy link
Collaborator

This PR adds the ability to put raw byte strings into the database. This PR covers the C++ and Python clients and Fortran and C clients will be addressed in a follow-up PR.

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 55.76923% with 92 lines in your changes missing coverage. Please review.

Project coverage is 57.58%. Comparing base (4937fbd) to head (aae3968).
Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
tests/cpp/unit-tests/test_client.cpp 0.00% 42 Missing ⚠️
tests/cpp/unit-tests/test_client_prefixing.cpp 0.00% 24 Missing ⚠️
src/python/src/pyclient.cpp 0.00% 12 Missing ⚠️
src/python/bindings/bind.cpp 0.00% 6 Missing ⚠️
src/cpp/client.cpp 92.06% 5 Missing ⚠️
src/python/module/smartredis/client.py 92.85% 2 Missing ⚠️
src/python/module/smartredis/error.py 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #521       +/-   ##
============================================
- Coverage    95.92%   57.58%   -38.34%     
============================================
  Files           10      147      +137     
  Lines          884    11907    +11023     
  Branches         0     1138     +1138     
============================================
+ Hits           848     6857     +6009     
- Misses          36     4899     +4863     
- Partials         0      151      +151     
Files with missing lines Coverage Δ
include/client.h 76.92% <ø> (ø)
include/redisserver.h 60.00% <ø> (ø)
src/cpp/redis.cpp 55.60% <100.00%> (ø)
src/cpp/rediscluster.cpp 66.75% <100.00%> (ø)
src/python/module/smartredis/error.py 95.45% <0.00%> (ø)
src/python/module/smartredis/client.py 97.28% <92.85%> (-0.28%) ⬇️
src/cpp/client.cpp 88.50% <92.06%> (ø)
src/python/bindings/bind.cpp 0.00% <0.00%> (ø)
src/python/src/pyclient.cpp 0.00% <0.00%> (ø)
tests/cpp/unit-tests/test_client_prefixing.cpp 0.00% <0.00%> (ø)
... and 1 more

... and 128 files with indirect coverage changes

Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Nicely done! One suggestion with regard to the unpack_bytes method in the C++ client


// Get byte data and fill an already allocated array
// memory space that has the specified size.
void Client::unpack_bytes(const std::string& name,
Copy link
Member

Choose a reason for hiding this comment

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

Currently this requires that the buffer and the reply from Redis are exactly the same size. This might be a little overly restrictive especially because the size of the byte string may not be known a priori. I'd suggest that we change this:

void Client::unpack_bytes(const std::string& name,
                          void* data,
                          const size_t n_buffer_size,
                          size_t* obj_size
)

The check internally should then be that the buffer size is >= to the message reply

Copy link
Member

@ashao ashao left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@mellis13 mellis13 merged commit 89103f2 into CrayLabs:develop Nov 4, 2024
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants