feat(server): implement a variant of ZUNION command

[ATM-SALEH]

This PR implements a variant of ZUNION command

Due to the lack of support in the framework, this implementation does not offer API to specify num_keys argument.

Instead, it currently can accept exactly 3 keys. All the optional arguments are supported (WEIGHTS, AGGREGATE, WITHSCORES)

(partially implements #356)

[ATM-SALEH]

@romange - Please have a look at your convenience

[romange]

Thanks, @ATM-SALEH we will take a look.

[ATM-SALEH]

Thanks a lot for your efforts!

The PR looks good in general. However, apart from two small issues, there is a lot of code nearly identical to ZUnionStore:

See my comments, but this is a brief conclusion:

• It would be desirable, that ParseUnionArgs should re-use some of the code for parsing StoreArgs and not do everything on its own.
• It would be great, if we could extend OpUnion just with a single flag to work for both cases.
• It would be remarkably great, if we could actually manage ZUnion and ZUnionStore with one function. This requires some basic knowledge of transactions, always feel free to ask any questions if you decide to take this up.

See for yourself, how much points on the list you want to handle. The last ones should be optional.

Thanks for the review!

About the duplicates between ZUNION and ZUNIONSTORE, I was also thinking along the line, however didn't specify anything here to have an unbiased review. I am happy to refactor in order to make the component more DRY.

I will start with Point 1 and 2 first. After that, I will review the transaction framework and start working on Point 3. Will reach out if there is any question

[ATM-SALEH]

Thanks a lot for your efforts!

The PR looks good in general. However, apart from two small issues, there is a lot of code nearly identical to ZUnionStore:

See my comments, but this is a brief conclusion:

• It would be desirable, that ParseUnionArgs should re-use some of the code for parsing StoreArgs and not do everything on its own.
• It would be great, if we could extend OpUnion just with a single flag to work for both cases.
• It would be remarkably great, if we could actually manage ZUnion and ZUnionStore with one function. This requires some basic knowledge of transactions, always feel free to ask any questions if you decide to take this up.

See for yourself, how much points on the list you want to handle. The last ones should be optional.

@dranikpg - Apologies for the delay!

I did some refactoring which should cover Point 1 and 2 as part of as part of https://github.com/dragonflydb/dragonfly/pull/420/commits/8c0866fea31cf04bad7e28fdd196e361aef27171. This commit should also include some changes related to other review comments. Please have a look at your convenience and provide feedback.

I will start looking for refactoring to cover Point 3

[ATM-SALEH]

Thanks a lot for your efforts! The PR looks good in general. However, apart from two small issues, there is a lot of code nearly identical to ZUnionStore: See my comments, but this is a brief conclusion:

• It would be desirable, that ParseUnionArgs should re-use some of the code for parsing StoreArgs and not do everything on its own.
• It would be great, if we could extend OpUnion just with a single flag to work for both cases.
• It would be remarkably great, if we could actually manage ZUnion and ZUnionStore with one function. This requires some basic knowledge of transactions, always feel free to ask any questions if you decide to take this up.

See for yourself, how much points on the list you want to handle. The last ones should be optional.

@dranikpg - Apologies for the delay!

I did some refactoring which should cover Point 1 and 2 as part of as part of 8c0866f. This commit should also include some changes related to other review comments. Please have a look at your convenience and provide feedback.

I will start looking for refactoring to cover Point 3

https://github.com/dragonflydb/dragonfly/pull/420/commits/e44d27b75530be3c97e787eca68957f7b1a3f088 contains changes related to Point 3.

[adiholden]

@ATM-SALEH thank you for your effort. Most of the comment I gave are for readability, the PR flow looks good

[romange]

Hey @ATM-SALEH how is it going? would you like us to take it from here or you think you will be able to finish the PR before 2023?

[ATM-SALEH]

Hey @ATM-SALEH how is it going? would you like us to take it from here or you think you will be able to finish the PR before 2023?

Apologies - busy weeks before holiday time.

I will try to allocate some time this week. By the weekend, I should be able to push the required changes / delegate this.

Is that ok?

[romange]

@ATM-SALEH absolutely, and you should not apologize. I appreciate the time you put into this project 🍻

[ATM-SALEH]

@ATM-SALEH absolutely, and you should not apologize. I appreciate the time you put into this project 🍻

@romange I think I would be a bit hard for me to allocate time for this PR this year. So, please feel free to take it from here if needed. Hope that's okay.

I am hoping to contribute more in the coming year to this interesting project

[romange]

Hi @ATM-SALEH no worries and thanks again! Would love to see you contributing to the project again!