Skip to content

sqlite: statement.setReadNullAsUndefined() #59462

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.

Sign up for GitHub

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

Open
jac0bandres wants to merge 6 commits into
base: main
Choose a base branch
from
+262 −20
Open

sqlite: statement.setReadNullAsUndefined() #59462

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c3702c
src: add setReadNullAsUndefined() to StatementSync in node_sqlite.cc …
jac0bandres Aug 13, 2025
f4a6327
Merge branch 'nodejs:main' into dev
jac0bandres Aug 13, 2025
06e050f
sqlite: add statement.setReadNullAsUndefined, add options.readNullAsU…
jac0bandres Aug 14, 2025
8af485a
sqlite: add statement.setReadNullAsUndefined, add options.readNullAsU…
jac0bandres Aug 14, 2025
f248a9e
sqlite: add statement.setReadNullAsUndefined, add options.readNullAsU…
jac0bandres Aug 14, 2025
b62aed9
sqlite: add statement.setReadNullAsUndefined, add options.readNullAsU…
jac0bandres Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions src/node_sqlite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ using v8::MaybeLocal;
using v8::Name;
using v8::NewStringType;
using v8::Null;
using v8::Undefined;
using v8::Number;
using v8::Object;
using v8::Promise;
Expand All @@ -67,7 +68,7 @@ using v8::Value;
} \
} while (0)

#define SQLITE_VALUE_TO_JS(from, isolate, use_big_int_args, result, ...) \
#define SQLITE_VALUE_TO_JS(from, isolate, use_big_int_args, use_null_as_undefined_, result, ...) \
do { \
switch (sqlite3_##from##_type(__VA_ARGS__)) { \
case SQLITE_INTEGER: { \
Expand Down Expand Up @@ -96,7 +97,11 @@ using v8::Value;
break; \
} \
case SQLITE_NULL: { \
(result) = Null((isolate)); \
if ((use_null_as_undefined_)) { \
(result) = Undefined((isolate)); \
} else { \
(result) = Null((isolate)); \
} \
Comment on lines +103 to +107
Copy link
Copy Markdown
Member

@H4ad H4ad Aug 13, 2025

Choose a reason for hiding this comment

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

@LiviaMedeiros when the value is undefined, should it be included as a key in the returned object?

V8 optimizes creation of the objects with similar shape but I don't know if this applies to objects created via C++ directly, if not, maybe could be worthy/faster just exclude this field.

Copy link
Copy Markdown
Member

@LiviaMedeiros LiviaMedeiros Aug 14, 2025

Choose a reason for hiding this comment

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

Performance-wise, i don't know. On huge amounts of rows and sparse data, i would expect hidden class to be faster at the cost of extra memory consumption; but without benchmarking it's just a guess.

However, this difference is observable in userland, which outweights microoptimizations. I think, explicit undefined is better:
If we get rows as objects using SELECT * FROM ..., we should be able to check what * was resolved to.
If we use returnArrays: true or statement.setReturnArrays(true) and keep it consistent with objects, omitting undefined means having sparse arrays which are counterintuitive.

If there is significant performance benefit from excluding fields, we can consider more explicit useNullAsEmpty. Or changing this from boolean to useNullAs(enum) while node:sqlite is still experimental.

break; \
} \
case SQLITE_BLOB: { \
Expand Down Expand Up @@ -310,7 +315,7 @@ class CustomAggregate {
for (int i = 0; i < argc; ++i) {
sqlite3_value* value = argv[i];
MaybeLocal<Value> js_val;
SQLITE_VALUE_TO_JS(value, isolate, self->use_bigint_args_, js_val, value);
SQLITE_VALUE_TO_JS(value, isolate, self->use_bigint_args_, false, js_val, value);
Comment thread
H4ad marked this conversation as resolved.
Outdated
if (js_val.IsEmpty()) {
// Ignore the SQLite error because a JavaScript exception is pending.
self->db_->SetIgnoreNextSQLiteError(true);
Expand Down Expand Up @@ -613,7 +618,7 @@ void UserDefinedFunction::xFunc(sqlite3_context* ctx,
for (int i = 0; i < argc; ++i) {
sqlite3_value* value = argv[i];
MaybeLocal<Value> js_val = MaybeLocal<Value>();
SQLITE_VALUE_TO_JS(value, isolate, self->use_bigint_args_, js_val, value);
SQLITE_VALUE_TO_JS(value, isolate, self->use_bigint_args_, false, js_val, value);
Comment thread
H4ad marked this conversation as resolved.
Outdated
if (js_val.IsEmpty()) {
// Ignore the SQLite error because a JavaScript exception is pending.
self->db_->SetIgnoreNextSQLiteError(true);
Expand Down Expand Up @@ -2000,7 +2005,7 @@ MaybeLocal<Value> StatementSync::ColumnToValue(const int column) {
Isolate* isolate = env()->isolate();
MaybeLocal<Value> js_val = MaybeLocal<Value>();
SQLITE_VALUE_TO_JS(
column, isolate, use_big_ints_, js_val, statement_, column);
column, isolate, use_big_ints_, use_null_as_undefined_, js_val, statement_, column);
return js_val;
}

Expand Down Expand Up @@ -2378,6 +2383,22 @@ void StatementSync::SetReadBigInts(const FunctionCallbackInfo<Value>& args) {
stmt->use_big_ints_ = args[0]->IsTrue();
}

void StatementSync::SetReadNullAsUndefined(const FunctionCallbackInfo<Value>& args) {
StatementSync* stmt;
ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
Environment* env = Environment::GetCurrent(args);
THROW_AND_RETURN_ON_BAD_STATE(
env, stmt->IsFinalized(), "statement has been finalized");

if (!args[0]->IsBoolean()) {
THROW_ERR_INVALID_ARG_TYPE(
env->isolate(), "The \"readNullAsUndefined\" argument must be a boolean.");
return;
}

stmt->use_null_as_undefined_= args[0]->IsTrue();
}

void StatementSync::SetReturnArrays(const FunctionCallbackInfo<Value>& args) {
StatementSync* stmt;
ASSIGN_OR_RETURN_UNWRAP(&stmt, args.This());
Expand Down Expand Up @@ -2447,6 +2468,8 @@ Local<FunctionTemplate> StatementSync::GetConstructorTemplate(
tmpl,
"setAllowUnknownNamedParameters",
StatementSync::SetAllowUnknownNamedParameters);
SetProtoMethod(
isolate, tmpl, "setReadNullAsUndefined", StatementSync::SetReadNullAsUndefined);
SetProtoMethod(
isolate, tmpl, "setReadBigInts", StatementSync::SetReadBigInts);
SetProtoMethod(
Expand Down
3 changes: 3 additions & 0 deletions src/node_sqlite.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ class DatabaseOpenConfiguration {
bool enable_dqs_ = false;
int timeout_ = 0;
bool use_big_ints_ = false;
bool use_null_as_undefined_ = false;
bool return_arrays_ = false;
bool allow_bare_named_params_ = true;
bool allow_unknown_named_params_ = false;
Expand Down Expand Up @@ -173,6 +174,7 @@ class StatementSync : public BaseObject {
static void SetAllowUnknownNamedParameters(
const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetReadBigInts(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetReadNullAsUndefined(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetReturnArrays(const v8::FunctionCallbackInfo<v8::Value>& args);
void Finalize();
bool IsFinalized();
Expand All @@ -186,6 +188,7 @@ class StatementSync : public BaseObject {
sqlite3_stmt* statement_;
bool return_arrays_ = false;
bool use_big_ints_;
bool use_null_as_undefined_ = false;
bool allow_bare_named_params_;
bool allow_unknown_named_params_;
std::optional<std::map<std::string, std::string>> bare_named_params_;
Expand Down
52 changes: 52 additions & 0 deletions test/parallel/test-sqlite-statement-sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,58 @@ suite('StatementSync.prototype.expandedSQL', () => {
});
});

suite('StatementSync.prototype.setReadNullAsUndefined', () => {
test('Null can be read as undefined', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
const setup = db.exec(`
CREATE TABLE data(is_null TEXT DEFAULT NULL) STRICT;
INSERT INTO data(is_null) VALUES(NULL);
`)
t.assert.strictEqual(setup, undefined);

const query = db.prepare('SELECT is_null FROM DATA');
t.assert.deepStrictEqual(query.get(),{ __proto__: null, is_null: null});
t.assert.strictEqual(query.setReadNullAsUndefined(true), undefined);
console.log(query.get());
Comment thread
H4ad marked this conversation as resolved.
Outdated
t.assert.deepStrictEqual(query.get(), { __proto__: null, is_null: undefined});
t.assert.strictEqual(query.setReadNullAsUndefined(false), undefined);
t.assert.deepStrictEqual(query.get(), { __proto__: null, is_null: null});
})

test('does not affect non-null values', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
const setup = db.exec(`
CREATE TABLE data(is_not_null TEXT) STRICT;
INSERT INTO data(is_not_null) VALUES('This is not null');
`)
t.assert.strictEqual(setup, undefined);

const query = db.prepare('SELECT is_not_null FROM DATA');
t.assert.deepStrictEqual(query.get(),{ __proto__: null, is_not_null: 'This is not null'});
t.assert.strictEqual(query.setReadNullAsUndefined(true), undefined);
t.assert.deepStrictEqual(query.get(), { __proto__: null, is_not_null: 'This is not null'});
})
test('throws when input is not a boolean', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
const setup = db.exec(`
CREATE TABLE data(is_not_null TEXT) STRICT;
INSERT INTO data(is_not_null) VALUES('This is not null');
`)
t.assert.strictEqual(setup, undefined);

const stmt = db.prepare('SELECT is_not_null FROM data');
t.assert.throws(() => {
stmt.setReadNullAsUndefined();
}, {
code: 'ERR_INVALID_ARG_TYPE',
message: /The "readNullAsUndefined" argument must be a boolean/,
});
});
});

suite('StatementSync.prototype.setReadBigInts()', () => {
test('BigInts support can be toggled', (t) => {
const db = new DatabaseSync(nextDb());
Expand Down