Improved match aggregate #4495
Improved match aggregate #4495
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4495 +/- ##
==========================================
+ Coverage 92.8% 92.85% +0.05%
==========================================
Files 118 118
Lines 8394 8442 +48
==========================================
+ Hits 7790 7839 +49
+ Misses 604 603 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Some small nits that don't necessarily need to be changed.
Beyond that the actual PG related code, according to the tests, seems to be good but I'll defer to someone for a proper analysis of that code.
You're probably ok with using the rest calls, ultimately so long as it still runs the same functionality internally.
|
|
||
| // Match view { $gt: 750, $lt: 850 } | ||
| expect(resp.results.some(result => result.score === 10)).toEqual(true); | ||
| expect(resp.results.some(result => result.views === 800)).toEqual(true); |
There was a problem hiding this comment.
You could just check that the value is equal to the expected value directly, rather than an equality check and bool check.
There was a problem hiding this comment.
Mongo and PG return different orders for results
If I check directly, one of the test would fail even tho they return correct results.
There was a problem hiding this comment.
If that's the case wouldn't this fail anyways with a comparison against a different result?
There was a problem hiding this comment.
.some() returns true if at least one element matches. So the order doesn’t matter.
There was a problem hiding this comment.
This also helps if future databases changes matches internally or query optimization returns different results order
There was a problem hiding this comment.
Ahhh, gotcha, alright we're good then.
| expect(results[0].pointer.objectId).toEqual(pointer1.id); | ||
| expect(results[1].pointer.objectId).toEqual(pointer1.id); | ||
| expect(results.some(result => result.objectId === obj1.id)).toEqual(true); | ||
| expect(results.some(result => result.objectId === obj3.id)).toEqual(true); |
There was a problem hiding this comment.
Same here, you can just compare that the expected objectId matches directly
|
@flovilmart I know you are a busy man. Can you look this over? 🙏 |
There was a problem hiding this comment.
It’s looking good, but the _id/objectId transformation is confusing as they are never object ID’s. It’s the primary key for the pipeline, so I guess we should keep it as _id.
However, when a user references the objectId key by putting $objectId, this should be transformed to $_id
|
@flovilmart I agree that can put either for the input but the output is already objectId |
|
Hi guys! Although presently I do not have time to be doing any support here, much less PR-s, I will just add that this library can perform better and have much simpler code, if it starts using the following important features of the
|
* Improve aggregate queries * using pg-promise query formatting * match multiple comparison * $or and complex match
Edit: Should I replace the REST calls with query.aggregate() in the tests?