sqlite: add aggregate function #56600
Conversation
nodejs-github-bot
added
c++
|
Also, tests and docs are needed. |
|
@geeksilva97 are you still planning to work on this? |
Hi @cjihrig . Yes, I plan to continue this. The last few weeks have been tough though. If nobody beats me I plan to get this done by April. |
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
7 times, most recently
from
beaf983 to
70dff34
Compare
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
from
70dff34 to
f4e4869
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56600 +/- ##
==========================================
- Coverage 90.23% 90.21% -0.03%
==========================================
Files 629 630 +1
Lines 184903 185388 +485
Branches 36223 36283 +60
==========================================
+ Hits 166853 167239 +386
- Misses 11010 11079 +69
- Partials 7040 7070 +30
🚀 New features to boost your workflow:
|
|
I got an implementation that works. I need to add tests and clean up the code. |
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
3 times, most recently
from
cd3f0aa to
22da3ad
Compare
geeksilva97
changed the title
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
from
22da3ad to
9badd9b
Compare
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
from
9badd9b to
7a0459a
Compare
| LocalVector<Value> js_argv(isolate); | ||
| js_argv.emplace_back(Local<Value>::New(isolate, agg->value)); | ||
|
|
||
| for (int i = 0; i < argc; ++i) { |
There was a problem hiding this comment.
I'm aware of this duplication. I have good expectations for #57571 and then this duplication will be resolved.
| THROW_ERR_INVALID_ARG_TYPE(env->isolate(), | ||
| "The \"options.start\" argument must be a " | ||
| "function or a primitive value."); |
There was a problem hiding this comment.
This error message doesn't seem good. Would anybody have a better suggestion?
options.start can be any valid JS value or a function that returns any JS value.
There was a problem hiding this comment.
In that case, maybe undefined is a valid value here?
geeksilva97
force-pushed
the
sqlite-aggregate-function
branch
from
7a0459a to
1f34c9e
Compare
|
Looks like the deterministic flag doesn't have an impact on aggregate functions (ref: https://sqlite.org/forum/info/10f2e7bcbdd5cfcac978e665815367aad175d407c19d9f9b4c1034f31b4a010a) I tried a couple of queries to get it under test but I couldn't. Maybe anyone in @nodejs/sqlite would know? (tagging since the team didn't exist when this PR was opened as a draft) |
geeksilva97
changed the title
There was a problem hiding this comment.
I gave this another quick skim, but I think we should wait for #57571 to land first before doing a more thorough review.
Looks like the deterministic flag doesn't have an impact on aggregate functions
We can probably drop it from the API then. If anyone ever comes along with a real use case (which there doesn't appear to be one), we can add it in the future.
| Local<Value> start_v = Local<Value>::New(isolate, start_); | ||
| if (start_v->IsFunction()) { | ||
| auto fn = start_v.As<Function>(); | ||
| TryCatch try_catch(isolate); |
There was a problem hiding this comment.
I don't think you need this TryCatches since it seems like it is only being used to detect pending exceptions, and that can be done without TryCatch.
| THROW_ERR_INVALID_ARG_TYPE(env->isolate(), | ||
| "The \"options.start\" argument must be a " | ||
| "function or a primitive value."); |
There was a problem hiding this comment.
In that case, maybe undefined is a valid value here?
Closes #56511
inspired by https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#aggregatename-options---this