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

Add function to allow std::variant of supported redis types #138

Closed
chill1n opened this issue Jan 3, 2021 · 12 comments
Closed

Add function to allow std::variant of supported redis types #138

chill1n opened this issue Jan 3, 2021 · 12 comments

Comments

@chill1n
Copy link

chill1n commented Jan 3, 2021

I have a specific problem with simply parsing the output of "memory stats" using the generic command("memory", "stats") call.
I've tried the obvious but it seems complicated with the current functionality.
Would it not be a generic extension to this library to add support for std::variant of supported redis types?
At this moment the current library supports STRING and INTEGER (not BOOL and DOUBLE yet), but that would suffice for most cases. An example output of "memory stats" is given below (which could be mapped to 25 pairs of either <string,long long> or <string,string>, or a single array of std::variant<string,long long>).
Any thoughts on how to approach parsing of "memory stats" with current functionality?
Any thoughts on the proposed support for std::variant?

  1. "peak.allocated"
  2. (integer) 18949144
  3. "total.allocated"
  4. (integer) 865720
  5. "startup.allocated"
  6. (integer) 802984
  7. "replication.backlog"
  8. (integer) 0
  9. "clients.slaves"
  10. (integer) 0
  11. "clients.normal"
  12. (integer) 16986
  13. "aof.buffer"
  14. (integer) 0
  15. "lua.caches"
  16. (integer) 0
  17. "overhead.total"
  18. (integer) 819970
  19. "keys.count"
  20. (integer) 0
  21. "keys.bytes-per-key"
  22. (integer) 0
  23. "dataset.bytes"
  24. (integer) 45750
  25. "dataset.percentage"
  26. "72.924636840820312"
  27. "peak.percentage"
  28. "4.5686497688293457"
  29. "allocator.allocated"
  30. (integer) 994000
  31. "allocator.active"
  32. (integer) 1363968
  33. "allocator.resident"
  34. (integer) 5042176
  35. "allocator-fragmentation.ratio"
  36. "1.3722012042999268"
  37. "allocator-fragmentation.bytes"
  38. (integer) 369968
  39. "allocator-rss.ratio"
  40. "3.6966967582702637"
  41. "allocator-rss.bytes"
  42. (integer) 3678208
  43. "rss-overhead.ratio"
  44. "0.63931763172149658"
  45. "rss-overhead.bytes"
  46. (integer) -1818624
  47. "fragmentation"
  48. "3.9087760448455811"
  49. "fragmentation.bytes"
  50. (integer) 2398856
@sewenew
Copy link
Owner

sewenew commented Jan 4, 2021

Would it not be a generic extension to this library to add support for std::variant of supported redis types?

Yes, it's great! I'll put it on the TODO list. Thanks for the advice!

the current library supports STRING and INTEGER (not BOOL and DOUBLE yet)

In fact, it also supports parsing the result to bool (integer reply that only has value 0 and 1) and double (string reply which can be converted to a double).

Any thoughts on how to approach parsing of "memory stats" with current functionality?

So far, you can do it in an ugly way, i.e. define a tuple which matches the output of memory stats:

// It's ugly, and you'd better double check if it matches
using Result = std::tuple<string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, long long, string, double, string, double, string, long long, string, long long, string, long long, string, double, string, long long, string, double, string, long long, string, double, string, long long, string, double, string, long long>;

auto result = redis.command<Result>("memory", "stats");

Thanks again for the proposal! If I have any progress on it, I'll let you know:)

Regards

@chill1n
Copy link
Author

chill1n commented Jan 4, 2021

Thanks for picking this up!

Good to hear that double and bool are also supported.
I appreciate the tuple suggestion. It's a nasty one, but if it does what I want it's good enough for the time being. However, I'm getting the following run-time error:

C++ exception with description "Expect tuple reply with 50elements" thrown in the test body.

Not sure what's wrong, as type/size mismatches seem relatively hard to debug. The data (size and types) seems to be correct in a gdb session. Another workaround that I've implemented in the mean time is through a Python wrapper function that returns a dictionary from which I can select and return the desired value(s). This call is not performance critical, so it's good enough.

@sewenew
Copy link
Owner

sewenew commented Jan 5, 2021

C++ exception with description "Expect tuple reply with 50elements" thrown in the test body.

That means the number of elements in the reply is NOT 50. You'd better use redis-cli to check the exact number and format of the memory stats command. My redis server (5.0.5) returns 52 elements, and one of which is a map<string, long long>:

17) "db.0"
18) 1) "overhead.hashtable.main"
    2) (integer) 12048
    3) "overhead.hashtable.expires"
    4) (integer) 0

Regards

@sewenew
Copy link
Owner

sewenew commented Jan 5, 2021

Also I've tried to implement parsing reply to variant. The code is on dev branch, and you can try it:

        using MyVar = Variant<long long, double>;
        auto r = Redis("tcp://127.0.0.1");
        auto v = r.command<unordered_map<string, MyVar>>("memory", "stats");
        for (const auto &ele : v) {
            cout << ele.first << "\t";
            visit([](auto &&arg) -> void { cout << arg << endl; }, ele.second);
        }

However, as I mentioned in previous comment, you need to check out the format of your redis server's output. If the result contains a unordered_map<string, long long>, you need to define MyVar as follows, and overload operator<< for the map.

using MyVar = Variant<long long, double, unordered_map<string, long long>>;

If you still have any problem, feel free to let me know.

Regards

@chill1n
Copy link
Author

chill1n commented Jan 5, 2021

C++ exception with description "Expect tuple reply with 50elements" thrown in the test body.

That means the number of elements in the reply is NOT 50. You'd better use redis-cli to check the exact number and format of the memory stats command. My redis server (5.0.5) returns 52 elements, and one of which is a map<string, long long>:

17) "db.0"
18) 1) "overhead.hashtable.main"
    2) (integer) 12048
    3) "overhead.hashtable.expires"
    4) (integer) 0

Regards

Interesting... I can reproduce your case with 52 response elements.
However, my initial post was based on actual copy&paste data too. So I found out that after a "flushall" command redis will return with 50 response elements.
This makes parsing this response a bit more tricky.
Would it be possible to have a std::variant type with a fallback type if mapping fails, e.g. nullptr? So in this case it could be a std::variant<string, long long, nullptr_t>

Any thoughts on this?

@chill1n
Copy link
Author

chill1n commented Jan 5, 2021

Also I've tried to implement parsing reply to variant. The code is on dev branch, and you can try it:

        using MyVar = Variant<long long, double>;
        auto r = Redis("tcp://127.0.0.1");
        auto v = r.command<unordered_map<string, MyVar>>("memory", "stats");
        for (const auto &ele : v) {
            cout << ele.first << "\t";
            visit([](auto &&arg) -> void { cout << arg << endl; }, ele.second);
        }

However, as I mentioned in previous comment, you need to check out the format of your redis server's output. If the result contains a unordered_map<string, long long>, you need to define MyVar as follows, and overload operator<< for the map.

using MyVar = Variant<long long, double, unordered_map<string, long long>>;

If you still have any problem, feel free to let me know.

Regards

Great! Will try asap.
Any thoughts about my previous proposal to ignore a mapping if the type does not match (and we don't care about that type)?

@chill1n
Copy link
Author

chill1n commented Jan 5, 2021

My testing shows that the implemented Variant functionality in the dev branch works as requested. Thanks a lot for the quick implementation!
Any thoughts on the "don't care" nullptr_t special case would still be appreciated though. It's a use case that matches well with std::variant. Moreover, it could improve performance and code complexity on the user side.

@chill1n
Copy link
Author

chill1n commented Jan 5, 2021

By the way, a more compact way to print a variant is;
get<0>(v)

@sewenew
Copy link
Owner

sewenew commented Jan 6, 2021

So I found out that after a "flushall" command redis will return with 50 response elements.

If there’s a db that is not empty, the command will show some stat on the db. That’s why you got 50 elements if you call flushall, and got 52 elements if you set some data to redis.

Any thoughts about my previous proposal to ignore a mapping if the type does not match (and we don't care about that type)?

Thanks for the suggestion! However, instead of nullptr_t, I’m thinking about using std::monostate as a placeholder for unknown types. That might be a c++ idiom. Still need some research. Thanks again!

Regards

@chill1n
Copy link
Author

chill1n commented Jan 7, 2021

std::variant is also new to me, as is std::monostate. Indeed std::monostate seems to be a more generic approach towards "don't care" types. Plus, you get the functionality for free with variant.

@sewenew
Copy link
Owner

sewenew commented Jan 10, 2021

@chill1n I refactor the code and add monostate support. The latest code has been merged into master branch. You can take a try :)

Regards

@chill1n
Copy link
Author

chill1n commented Jan 10, 2021

Thanks for the quick resolution. I've tested the master branch and can confirm that variant with monostate is working as expected. I will close this issue.

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

No branches or pull requests

2 participants