-
Notifications
You must be signed in to change notification settings - Fork 409
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
Implement $expr
evaluation query operator
#3163
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3163 +/- ##
==========================================
+ Coverage 75.37% 75.42% +0.05%
==========================================
Files 395 396 +1
Lines 21953 22202 +249
==========================================
+ Hits 16546 16746 +200
- Misses 4436 4474 +38
- Partials 971 982 +11
... and 10 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
I like the test cases a lot! It looks to me that all the important paths are covered. I asked a question about the part I don't understand.
$expr
query operator$expr
evaluation query operator
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.
LGTM
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.
LGTM!
"go.mongodb.org/mongo-driver/bson" | ||
) | ||
|
||
func TestAggregateCompatMatchExpr(t *testing.T) { |
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.
Could someone explain me the reason for that test, given that db.col.aggregate({$match: {$expr: X}})
is exactly the same as db.col.aggregate(X)
for any X?
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.
@AlekSi do you have a particular example? I'm afraid I have to disagree with your current statement, at least because the second part db.col.aggregate(X)
should include an aggregation pipeline stage, and X
is not an aggregation pipeline stage but an aggregation expression. Your first statement is not entirely true either because aggregate
needs an array of stages, not an object.
Regarding the $match
stage, see also: https://www.mongodb.com/docs/manual/reference/operator/aggregation/match/#restrictions.
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.
Do you mean db.col.aggregate([{$match: {$expr: X}}])
and db.col.find({$expr: X})
are the same? Yes they are the same as far as those tests show, I didn't know it until I added those tests.
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.
All that goes back to my message in Slack. The fact that we try to get an understanding from examples rather than from fundamentals is even more concerning.
$match
is the aggregation stage that uses query operators for filtering. $expr
is a query operator that uses aggregation expressions. They mirror each other pretty much by definition. That test pretty much tests that (√x)² == x
for 20 normal values of x. I'm not saying that we should not test it for one normal x and some edge cases. But we already have a problem with slow tests that no amount of engineering could solve if we keep adding tautologous tests.
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.
Yes I am aware they mirror the behavior and that tests do test them both. Okie, I'm happy with reducing the number of tests by only testing few scenarios of the other one.
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.
I totally agree with Chi. We don't know if things are equal unless we test them. Especially, in case of error codes, error messages, etc. We have already had examples in the past when we thought things work in a way A (because documentation was silent about it), but it turned out in reality they work in a way B. I understand a concern regarding the number of tests, if we are ready for potential incompatibility, let's reduce the tests, it's never a problem to delete something.
Regarding "who uses $match
in aggregation when it's possible to use find
" - well, I guess someone who needs more than one stage in the aggregation pipeline. E.g. if after $match
I want to group, or project, or something, I would prefer to have $match
with $expr
, not find
and manual processing of data.
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.
For the case of $match
stage and find
command, it's safe to remove many of those tests, we've validated that they are the same. So let's remove those duplicated ones and not hinder the performance 🤗
During the implementation though, those tests were still helpful and it doesn't hurt to test them. But we don't need them anymore 🤗
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.
We don't know if things are equal unless we test them
The fact that MongoDB has some unexpected behavior in some edge cases does not mean that everything is an edge case. We don't test 2+2 and then test 2+3 to check if they behave differently; instead, we test 2+100500 because we suspect the overflow. So why do we test true
and then test false
without testing null
, for example?
it's never a problem to delete something
That requires much more thinking and analysis after the fact – is it safe to delete some tests? does it test some edge cases? why this test exists in the first place? It is not obvious that the test exists for no good reason. That's why we should not add them in the first place.
Regarding "who uses $match in aggregation when it's possible to use find"
I did not say that, though.
Description
Closes #414.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Labels, Project and project's Sprint fields.