-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
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.
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
base: main
Are you sure you want to change the base?
Conversation
…and node_sqlite.h In reference to the feature request nodejs#59457. setReadNullAsUndefined(boolean) converts SQL NULL values to JS undefined. Tests added to test/test-sqlite-statement-sync.js Fixes: nodejs#59457
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks solid, thanks for contributing! I added one comment (about a console log) and the other are more to discuss rather than change something.
Edit: the commit probably should something like "sqlite: add statement.setReadNullAsUndefined"
if ((use_null_as_undefined_)) { \ | ||
(result) = Undefined((isolate)); \ | ||
} else { \ | ||
(result) = Null((isolate)); \ | ||
} \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@H4ad Thanks for looking at my PR! I've implemented the changes you suggested, Tests have been written for respective classes in test/parallel/test-sqlite-aggregate-function.mjs and test/parallel/test-sqlite-database-sync.js. @LiviaMedeiros this means database-wide option.readNullAsUndefined is an option. Let me know how it looks. I'll get started on some documentation afterwards. |
What's the usage case here? Is this adding anything beyond changing what goes on the right-hand-side of a The bigint option is a genuine behavioural change for large integers that can only be implemented at library level, whereas this seems rather arbitrary. |
You bring up a good point.
This feature is implemented at a much lower level, within the V8 binding. A userland solution requires looping over results and manually manipulating objects in the runtime. This quickly becomes expensive over several iterations and bug prone if you're not careful. Or the === check could be enough if the function implementing it becomes hot enough for the optimizer. Otherwise this feature introduces more intentional and faster data type handling out the box. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jac0bandres thanks for working on this!
The code looks good, aside from a few linter complains (you can run make lint
locally to see them).
It would be nice to check that property with undefined
value is actually set (using in
or Object.hasOwn()
or Object.keys()
) to the tests; and test that this option works together with returnArrays
option and statement.setReturnArrays()
.
t.after(() => { db.close(); }); | ||
const bad = db.prepare(`SELECT ${Number.MAX_SAFE_INTEGER} + 1`); | ||
t.after(() => { db.close(); }); const bad = db.prepare(`SELECT ${Number.MAX_SAFE_INTEGER} + 1`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks unrelated.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59462 +/- ##
==========================================
- Coverage 89.88% 89.85% -0.04%
==========================================
Files 656 656
Lines 192967 193006 +39
Branches 37848 37854 +6
==========================================
- Hits 173451 173428 -23
- Misses 12070 12111 +41
- Partials 7446 7467 +21
🚀 New features to boost your workflow:
|
@LiviaMedeiros Thanks for the test suggestions! The undefined option is set and checked with |
This change is in reference to this feature request: #59457.
It adds setReadNullAsUndefined(boolean) so StatementSync in src/node_sqlite.cc and src/node_sqlite.h. All SQL null types will be converted to JavaScript undefined. This is particularly useful for accommodating the null/undefined JavaScript quirk.
A test for setReadNullAsUndefined is written into test-sqlite-statement-sync.js.