[TYPES] Implement Int128, Int256

Eventual-Inc / Daft

Distributed DataFrame for Python designed for the cloud, powered by Rust

https://getdaft.io

Apache License 2.0

1.82k stars 113 forks source link

[TYPES] Implement Int128, Int256 #935

Closed samster25 closed 11 months ago

samster25 commented 1 year ago

Implement Int128 and Int256 to unblock decimal implementation: https://github.com/Eventual-Inc/Daft/issues/934

mehtamohit013 commented 1 year ago

Hi @samster25, Is this issue available to work on? I would like to work on this.

Also, are there any contributing guidelines or pointers, which the PR and the code should conform to?

Thank you

xcharleslin commented 1 year ago

Hey @mehtamohit013, no one is working on this yet. We do hope to close this issue within the next few days since it's blocking the Decimal type, so if it isn't closed soon I will work on it.

The contribution guidelines are here: https://github.com/Eventual-Inc/Daft/blob/main/CONTRIBUTING.md, and there is a pre-commit hook for basic style checks.

mehtamohit013 commented 1 year ago

Thanks @xcharleslin

I will try my best to close this in a few days.

mehtamohit013 commented 1 year ago

Hi @xcharleslin,

As far as I understand, arrow2 doesn't support Int128 or Int256. It instead supports Decimal and Decimal256. So, how I should go along implementing Int128, using ArrowType::Decimal(32,32)?
In /src/datatype/mod.rs, on which datatype, should I use the trait NumericNative, as NativeArthimetics doesn't support primitive type i128(Docs)?
We might not need Int128 or Int256 to implement Decimal and Decimal256 (Not sure)

PS: I am new to Rust, so I might miss something very simple.

Thanks

samster25 commented 1 year ago

Hi @mehtamohit013!

Thanks for looking into it! If that is the case that arrow2 doesn't support Int128 and Int256. It's probably okay to just implement Decimal and Decimal256 and skip the Int* It looks like arrow2 should support the NativeType trait as seen here: https://docs.rs/arrow2/latest/arrow2/types/trait.NativeType.html#foreign-impls.

One thing you can do is make a PR and link it here and we can work with you to make it work :)

xcharleslin commented 1 year ago

Good catch! So it seems arrow2 does implement Int128 and Int256; however, they are not part of the Apache Arrow spec, and they don't exist in PyArrow either.

Agree with @samster25 , let's just implement the Decimal* types instead, which are indeed part of the Arrow spec.

xcharleslin commented 11 months ago

@mehtamohit013 let us know if there are any updates here. If not, I will begin working on this issue soon

mehtamohit013 commented 11 months ago

Hi @xcharleslin, Sorry, I didn't manage to spare some time for the issue. Feel free to go ahead to work on it.

Thanks

xcharleslin commented 11 months ago

Folding into #934. Int128 is done (but exposed only internally for Decimal128).