RedisLabsModules / redex

Extension modules to Redis' native data types and commands
GNU Affero General Public License v3.0
69 stars 13 forks source link

issue#2: Union multiple sorted sets with top K elements returned #3

Closed levyfan closed 8 years ago

levyfan commented 8 years ago

2 Union multiple sorted sets with top K elements returned

See details: https://groups.google.com/forum/#!topic/redis-dev/gTX6Jt2gr3s

Proposed command: ZUNIONMINK k numkeys key [key ...] [WEIGHTS weight [weight ...]] [WITHSCORES] ZUNIONMAXK k numkeys key [key ...] [WEIGHTS weight [weight ...]] [WITHSCORES]

heap.c is a implementation based on Vector. The interface keeps close to std::make_heap, std::pop_heap and std::push_heap.

Tested on redis master(unstable) branch.

itamarhaber commented 8 years ago

@levyfan thank you - reviewing.

itamarhaber commented 8 years ago

@levyfan looks excellent. A few things:

dvirsky commented 8 years ago

@levyfan all in all an excellent PR. we are very excited about it! Are you planning on writing additional modules yourself?

levyfan commented 8 years ago

@dvirsky I am planing to write a feed system just like facebook. Our team all agree that redis is a very good start point. We will write many business codes on redis and we are happy to share our work if it (or some part) could be generalized.

levyfan commented 8 years ago

Hi @itamarhaber , please have a review. And I also create a PR including the heap implementation at https://github.com/RedisLabs/RedisModulesSDK/pull/3

itamarhaber commented 8 years ago

Thanks once again @levyfan - looks awesome!

The only remark I have is that the getkeys-api is declared during command creation but the implementation itself is missing from the command.

levyfan commented 8 years ago

Hi @itamarhaber , I see a TODO at rxsets.c /* TODO: handle this once the getkey-api allows signalling errors */ It is only allowed to return REDISMODULE_OK if it is a getkey-api call, right?

levyfan commented 8 years ago

@itamarhaber I write something like return RedisModule_IsKeysPositionRequest(ctx) ? REDISMODULE_OK : RedisModule_WrongArity(ctx); just as rxsets.c did. Please have a review.

itamarhaber commented 8 years ago

Yep - that's the best I could come up with ;)

Thank you so much, I'm really looking forward to seeing what you and your team will next come up with - merging.

itamarhaber commented 8 years ago

Fixes #2