neophenix / Redis-hiredis

Perl wrapper around hiredis client
BSD 3-Clause "New" or "Revised" License
11 stars 8 forks source link

Support for redisearch? #10

Open lumina7 opened 4 years ago

lumina7 commented 4 years ago

Hello, great module :)

Redisearch command works but some of the results are marked as 'undef'.

https://oss.redislabs.com/redisearch/Quick_Start.html

git clone --recursive https://github.com/RediSearch/RediSearch.git
cd RediSearch
make build
make run
use Redis::hiredis;
use Data::Dumper;

my $redis = Redis::hiredis->new();
   $redis->connect('127.0.0.1', 6379);

# create index
$redis->command("FT.CREATE my_index SCHEMA title TEXT body TEXT");

# add documents
$redis->command("FT.ADD my_index doc1 1.0 FIELDS title foo body bar");
$redis->command("FT.ADD my_index doc2 1.0 FIELDS title foo_2 body bar_2");

# search
my $search = $redis->command("FT.SEARCH my_index foo");

print Dumper(\$search);

$VAR1 = \[
            1,
            'doc1',
            undef
          ];

On redis-cli I get.

1) (integer) 1
2) "doc1"
3) 1) "title"
   2) "foo"
   3) "body"
   4) "bar"

Looks like some array are not getting parsed. Redisearch is pretty great and we have 0 perl client, a fix would be appreciated.

neophenix commented 4 years ago

hiredis isn't even listed as a client on the redisearch site so not sure if this is something I can do or not. This lib is just a wrapper on that, so if it doesn't support it this lib wouldn't either. Not sure when I would be able to try it, but the most I can really hope for here is that an upgrade of the hiredis lib leads to it "just working"

lumina7 commented 4 years ago

Thank you. As this works on redis-cli, the last version of hiredis should work "finger crossed"

This really looks like a very trivial fix but I am totally clueless with C.

lumina7 commented 4 years ago

Redisearch do work with hiredis (v0.14.0) and an older one (v0.11.0). 0.11 is very old, so I don't think upgrading will have any change. I could not figure which one was bundled with the package.

a gist of working hiredis with redisearch: https://gist.github.com/michael-grunder/0a6a49b39660c9d55e13c551ca78b2ee

The problem probably comes because the redisearch reponse is a mix of array of array and array elements, and that usually never happen in classic redis.

Returns¶ Array reply, where the first element is the total number of results, and then pairs of document id, and a nested array of field/value.

lumina7 commented 4 years ago

The module (at least the one in cpan) is using hiredis 0.11.0, which is working with redisearch, the issue is with the wrapper...

https://metacpan.org/source/NEOPHENIX/Redis-hiredis-0.11.0/hiredis.h#L38

neophenix commented 4 years ago

Well for whatever reason I didn't see your replies about why it wasn't working and went ahead with the hiredis upgrade tonight.

Honestly, I am not sure I really have the interest in digging much further into it since I no longer use Perl or Redis. I'd gladly merge a PR if the tests are passing, but beyond that I really don't see me doing much with it, sorry.

Edit: however, the file you linked to is not my code, that is taken from hiredis, so maybe this will work, I'm interested to know since I did not test this specific use, just made sure make test still worked