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

[Part 2] nodejs-polars fails on supported NAPI function #2056

Closed
controversial opened this issue Feb 12, 2023 · 3 comments · Fixed by #2061
Closed

[Part 2] nodejs-polars fails on supported NAPI function #2056

controversial opened this issue Feb 12, 2023 · 3 comments · Fixed by #2061
Assignees
Labels
bug Something isn't working napi Compatibility with the native layer of Node.js

Comments

@controversial
Copy link
Contributor

What version of Bun is running?

0.5.7 (8b4e58f)

What platform is your computer?

Darwin 22.3.0 arm64 arm

What steps can reproduce the bug?

The following script attempts to use the nodejs-polars package, which is the official bindings (using napi-rs) to the Polars dataframe library.

The following simple script attempts to create a dataframe:

import pl from 'nodejs-polars';

const df = pl.DataFrame([
  { a: 1, b: 2 },
  { a: 4, b: 10 },
]);
console.log(df);

What is the expected behavior?

Using Node:

❯ node test.js
shape: (2, 2)
┌─────┬──────┐
│ a   ┆ b    │
│ --- ┆ ---  │
│ f64 ┆ f64  │
╞═════╪══════╡
│ 1.0 ┆ 2.0  │
├╌╌╌╌╌┼╌╌╌╌╌╌┤
│ 4.0 ┆ 10.0 │
└─────┴──────┘

What do you see instead?

Using Bun:

❯ bun test.js
125 |                 try {
126 |                     if (localFileExisted) {
127 |                         nativeBinding = require('./nodejs-polars.darwin-arm64.node');
128 |                     }
129 |                     else {
130 |                         nativeBinding = require('nodejs-polars-darwin-arm64');
                            ^
TypeError: dlopen(/Users/luke/Developer/dataset/node_modules/nodejs-polars-darwin-arm64/nodejs-polars.darwin-arm64.node, 0x0001): symbol not found in flat namespace '_napi_get_value_bigint_words'
      at /Users/luke/Developer/dataset/node_modules/nodejs-polars/bin/native-polars.js:130:24
      at /Users/luke/Developer/dataset/node_modules/nodejs-polars/bin/internals/polars_internal.js:6:27
      at /Users/luke/Developer/dataset/node_modules/nodejs-polars/bin/series/series.js:7:26
      at /Users/luke/Developer/dataset/node_modules/nodejs-polars/bin/index.js:24:15
      at /Users/luke/Developer/dataset/test.js:1:0

Additional information

The napi tracking issue says napi_get_value_bigint_words should be supported.

This issue is an exact duplicate of #2053, for a different NAPI function.

@controversial controversial added the bug Something isn't working label Feb 12, 2023
@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Feb 12, 2023

The #158 says napi_get_value_bigint_words should be supported.

Yeah...I remember implementing this. But I don't see the code.

Edit: Oh, the one I remember was this which is not exactly the same

extern "C" napi_status napi_create_bigint_words(napi_env env,
int sign_bit,
size_t word_count,
const uint64_t* words,
napi_value* result)
{
Zig::GlobalObject* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
auto* bigint = JSC::JSBigInt::tryCreateWithLength(vm, word_count);
if (UNLIKELY(!bigint)) {
return napi_generic_failure;
}
// TODO: verify sign bit is consistent
bigint->setSign(sign_bit);
if (words != nullptr) {
const uint64_t* word = words;
// TODO: add fast path that uses memcpy here instead of setDigit
// we need to add this to JSC. V8 has this optimization
for (size_t i = 0; i < word_count; i++) {
bigint->setDigit(i, *word++);
}
}
*result = toNapi(bigint);
return napi_ok;
}

@controversial
Copy link
Contributor Author

If it’s not implemented, #158 should probably be updated?

@ThatOneBro
Copy link
Contributor

I'll get this fixed really quick today. We should probably double check to make sure all of the list is up to date though

@ThatOneBro ThatOneBro self-assigned this Feb 13, 2023
@Electroid Electroid added the napi Compatibility with the native layer of Node.js label Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working napi Compatibility with the native layer of Node.js
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants