#143: Support: Byte Array DataType and Operations

DiceDB / dice

DiceDB is hyper-optimized for building and scaling truly real-time applications on modern hardware. It is a drop-in replacement for Redis with support for SQL-based reactivity.

https://dicedb.io/

Other

3.32k stars 420 forks source link

#143: Support: Byte Array DataType and Operations #180

Closed MohitVachhani closed 1 month ago

MohitVachhani commented 1 month ago

This PR addresses [Issue]

New data type: ByteArray
- Introduced a length property inside ByteArray to be used whenever we call len().
- Clearing the data of ByteArray if we decrease its size! [Example]
Commands like SETBIT/GETBIT/BITCOUNT/BitOp
- With unit Test cases.
- With benchmark test cases.

Note:

I have not addressed the BITOP operation in this PR because it is already quite large.

arpitbbhayani commented 1 month ago

@yashs360, need your help here. I am in a dilemma.

Given Redis is in C, strings are essentially bytes. In Go, strings are strings and any byte level operation becomes costly. I was thinking that we diverge from Redis here and offer bit operations on only ByteArray type that you introduced and raise error in case someone is doing it on other data type.

What do you think?

FYI @JyotinderSingh

MohitVachhani commented 1 month ago

Hey @arpitbbhayani / @JyotinderSingh / @yashs360

I have added support for:

New data type: ByteArray
Commands like SETBIT/GETBIT
ByteArray usage inside evalBITCOUNT

Doubts:

Should we support the string data type for SETBIT/GETBIT commands?
ByteArray:
- Should I introduce a length property inside ByteArray to be used whenever we call len()?
- This property would be maintained whenever we increase the array size.
- Do we need to clear the data of ByteArray if we decrease its size?
- For example, when reducing the size of the array, should we explicitly clear the truncated data?
```
SETBIT k 7 1
SETBIT K 23 1 
SETBIT K 23 0
```
So while doing SETBIT K 23 0, should we free up the memory, they are doing in Redis -> Code Reference: This

yashs360 commented 1 month ago

@arpitbbhayani @MohitVachhani

offer bit operations on only ByteArray

I think its fine as long as we document it. We should be able to diverge from redis as long as we have the capability covered by a the byte commands, and fail for divergence scenarios.

MohitVachhani commented 1 month ago

Okay @yashs360 , I will try to add in the comments itself and also will throw different error if any bit operations are done on strings 👍🏾

yashs360 commented 1 month ago

Nice change @MohitVachhani . Dropped a few comments.

MohitVachhani commented 1 month ago

Hey @yashs360 / @JyotinderSingh,

Could you please let me know the next steps here ? I’m looking to complete this task.

Thanks 🙇🏾

yashs360 commented 1 month ago

@MohitVachhani Overall the he change looks good, but the rewrite of Set/Get Bits doesn't make sense to me. If there are concerns with existing functions we should modify the existing code to address that instead of creating new functions. Could you please look at that.

MohitVachhani commented 1 month ago

Sure will update the existing function and let you know soon @yashs360

MohitVachhani commented 1 month ago

Hey @yashs360 I have updated the code, as per the above comments. Please can you check it out. Thanks 🙇🏾

JyotinderSingh commented 1 month ago

Thanks for the review @yashs360

JyotinderSingh commented 1 month ago

@MohitVachhani please fix the linter issues. Also added a minor comment.

JyotinderSingh commented 1 month ago

Thanks for addressing the comments and working on this large PR, merged!