Skip to content

Basic UDF support for Node API - #3092

Merged
hannes merged 18 commits into
duckdb:masterfrom
hannes:nodeudf
Feb 24, 2022
Merged

hannes merged 18 commits into
duckdb:masterfrom
hannes:nodeudf

Conversation

@hannes

@hannes hannes commented Feb 15, 2022

Copy link
Copy Markdown
Member

This PR adds basic JS UDFs for the Node API. Here is a minimal example:

db = new duckdb.Database(':memory:');
db.register("my_udf", "integer", (x) => x+1);
db.all("select my_udf(42) v", function(err, rows) {
   console.log(rows[0]);
});
db.unregister("my_udf");

@samansmink samansmink left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My node API experience is very limited but I added two small comments. The rest looks good to me!

Comment thread tools/nodejs/src/database.cpp Outdated
Comment thread tools/nodejs/src/connection.cpp
@hannes

hannes commented Feb 15, 2022

Copy link
Copy Markdown
Member Author

I seem to have broken something wrt prepare(). Will have a look tomorrow.

Comment thread tools/nodejs/lib/duckdb.js Outdated
for (let i = 0; i < descr.rows; ++i) {
const res = fun();
out_data[i] = res;
out_validity[i] = res == undefined ? 0 : 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer null over undefined here as return type for nulls.
Maybe res === undefined || res === null ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens if we get undefined then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe out_validity[i] = (res == undefined || res === null)? 0 : 1; ?
I just thought that supporting an actual null as return value might be semantically better.

@hannes

hannes commented Feb 23, 2022

Copy link
Copy Markdown
Member Author

The Node 14 crash seems related to node-ffi-napi/ref-napi#54

Comment thread Makefile
python3 scripts/format.py --all --check --silent

format-fix:
rm -rf src/amalgamation/*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these files not be ignored by the format script already?

include_directories(/usr/local/lib/node_modules/node-addon-api/)
include_directories(/usr/local/Cellar/node/14.12.0/include/node/)
include_directories(/opt/homebrew/lib/node_modules/node-addon-api)
include_directories(/opt/homebrew/Cellar/node/17.4.0/include/node)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way of getting these include directories?

@hannes
hannes merged commit fbe58d3 into duckdb:master Feb 24, 2022
@hannes
hannes deleted the nodeudf branch March 1, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants