Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Decimal type #210

Closed
wants to merge 5 commits into from
Closed

Adding Decimal type #210

wants to merge 5 commits into from

Conversation

Bidek56
Copy link
Collaborator

@Bidek56 Bidek56 commented May 20, 2024

Adding Polars Decimal type, although this functionality is considered unstable in Polars ATM. To close #209

@Bidek56
Copy link
Collaborator Author

Bidek56 commented May 20, 2024

@universalmind303 Would you know how to fix this method for Decimal type? Thx

impl FromNapiValue for Wrap<ChunkedArray<Int128Type>> {
    unsafe fn from_napi_value(env: sys::napi_env, napi_val: sys::napi_value) -> JsResult<Self> {
        let arr = Array::from_napi_value(env, napi_val)?;
        let len = arr.len() as usize;
        let mut builder = PrimitiveChunkedBuilder::<Int128Type>::new("", len);
        for i in 0..len {
            match arr.get::<BigInt>(i as u32) {
                Ok(val) => match val {
                    Some(v) => {
                        let (v, _b) = v.get_i128();
                        builder.append_value(v)
                    }
                    None => builder.append_null(),
                },
                Err(_) => {
                    builder.append_null()
                }
            }
        }
        Ok(Wrap(builder.finish()))
    }
}

@Bidek56 Bidek56 self-assigned this May 20, 2024
@Bidek56 Bidek56 added the enhancement New feature or request label May 20, 2024
@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jun 10, 2024

@flisky Would you know how to fix this method for Decimal type? Thx

@flisky
Copy link

flisky commented Jun 11, 2024

You have to keep scale at least, although the type declared as Option<usize>, it's required actually. The Option is just user faced, and it should be resolved as soon as possible.

@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jun 11, 2024

@stinodego Would you know how to fix this method for Decimal type? Thx

@stinodego
Copy link
Member

I don't know, but looks like you're not currently implementing it for Decimal. You're implementing on Int128 when you should be implementing on Logical<DecimalType, Int128Type>.

@Bidek56 Bidek56 closed this Jun 12, 2024
@Bidek56 Bidek56 deleted the issue-209 branch June 12, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Decimal DataType
3 participants