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

Upgrading to rs 0.41.3 #238

Merged
merged 7 commits into from
Jul 9, 2024
Merged

Upgrading to rs 0.41.3 #238

merged 7 commits into from
Jul 9, 2024

Conversation

Bidek56
Copy link
Collaborator

@Bidek56 Bidek56 commented Jul 3, 2024

Upgrading to rs-0.41.3

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

Bidek56 commented Jul 3, 2024

@universalmind303
The only missing test from this PR is expr.test.ts:expr.slice
If you have any ideas how to fix polars/lazy/expr/slice pls LMK thx.

package.json Outdated Show resolved Hide resolved
@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jul 3, 2024

Any ideas how to convert this line: ValueType::Number => AnyValue::Float64(f64::from_napi_value(env, napi_val)?),
to return u32 or f64 ? Thx
Apparently it's not an easy task according to SO.

@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jul 5, 2024

Is this conversion from number to either float or int too dangerous of an assumption? It works, but....

ValueType::Number => { 
    let napi_float = f64::from_napi_value(env, napi_val)?;
    if napi_float.fract() != 0.0 {
        AnyValue::Float64(napi_float)
    } else {
        AnyValue::Int32(i32::from_napi_value(env, napi_val)?)                    
    }
},

@universalmind303
Copy link
Collaborator

Any ideas how to convert this line: ValueType::Number => AnyValue::Float64(f64::from_napi_value(env, napi_val)?), to return u32 or f64 ? Thx Apparently it's not an easy task according to SO.

why can't it just continue to always return f64?

@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jul 5, 2024

Any ideas how to convert this line: ValueType::Number => AnyValue::Float64(f64::from_napi_value(env, napi_val)?), to return u32 or f64 ? Thx Apparently it's not an easy task according to SO.

why can't it just continue to always return f64?

Because rs-0.41.3 has introduced a int check for the slice method parameters. Passing f64 causes an error.

@Bidek56
Copy link
Collaborator Author

Bidek56 commented Jul 6, 2024

Conversion from f64 to i32:

ValueType::Number => { 
    let napi_float = f64::from_napi_value(env, napi_val)?;
    if napi_float.fract() != 0.0 {
        AnyValue::Float64(napi_float)
    } else {
        AnyValue::Int32(i32::from_napi_value(env, napi_val)?)                    
    }
},

would involve changing this as well:

 case "number":
      return Number.isInteger(value) ? DataType.Int32 : DataType.Float64;

and it would break a number of tests like this one:

    Expected: 
    >>shape: (3, 1)
    ┌───────────┐
    │ eq        │
    │ ---       │
    │ str       │
    ╞═══════════╡
    │ a=a;b=1.0 │
    │ a=b;b=2.0 │
    │ a=c;b=3.0 │
    └───────────┘ 
    Received:
    >>shape: (3, 1)
    ┌─────────┐
    │ eq      │
    │ ---     │
    │ str     │
    ╞═════════╡
    │ a=a;b=1 │
    │ a=b;b=2 │
    │ a=c;b=3 │
    └─────────┘

@universalmind303 Pls LMK how to do you want to proceed. Thx

@universalmind303
Copy link
Collaborator

@Bidek56 can't we just cast from f64 to an accepted type

#[napi(catch_unwind)]
    pub fn slice(&self, offset: &JsExpr, length: &JsExpr) -> JsExpr {
        self.inner
            .clone()
            .slice(offset.inner.clone().cast(DataType::Int64), length.inner.clone().cast(DataType::Int64))
            .into()
    }

@Bidek56 Bidek56 marked this pull request as ready for review July 7, 2024 10:22
@universalmind303 universalmind303 merged commit 67285ed into pola-rs:main Jul 9, 2024
9 checks passed
@Bidek56 Bidek56 deleted the rs-0.41.3 branch July 9, 2024 22:07
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.

None yet

2 participants