Skip to content
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

Merged
merged 23 commits into from
Aug 9, 2023

Conversation

chilagrow
Copy link
Member

@chilagrow chilagrow commented Aug 4, 2023

Description

Closes #414.

Readiness checklist

  • I added/updated unit tests (and they pass).
  • I added/updated integration/compatibility tests (and they pass).
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@chilagrow chilagrow added the code/feature Some user-visible feature is not implemented yet label Aug 4, 2023
@chilagrow chilagrow self-assigned this Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #3163 (6959e53) into main (b2aa1d3) will increase coverage by 0.05%.
The diff coverage is 78.98%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Changed Coverage Δ
...andlers/common/aggregations/operators/operators.go 74.46% <66.66%> (-0.54%) ⬇️
...nal/handlers/common/aggregations/operators/expr.go 76.81% <76.81%> (ø)
internal/handlers/common/filter.go 84.10% <78.26%> (-0.12%) ⬇️
...nternal/handlers/common/aggregations/expression.go 90.58% <100.00%> (+0.22%) ⬆️
.../common/aggregations/operators/operators_errors.go 100.00% <100.00%> (ø)
...nal/handlers/common/aggregations/operators/type.go 80.00% <100.00%> (+0.28%) ⬆️
...ernal/handlers/common/aggregations/stages/match.go 100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

Flag Coverage Δ
hana ?
integration 71.82% <78.98%> (+0.12%) ⬆️
mongodb 5.30% <0.00%> (-0.07%) ⬇️
pg 65.20% <78.98%> (+0.11%) ⬆️
shard-1 55.41% <4.66%> (-1.97%) ⬇️
shard-2 54.84% <78.59%> (+1.75%) ⬆️
shard-3 56.81% <5.44%> (+1.37%) ⬆️
sqlite 37.00% <40.07%> (-1.10%) ⬇️
unit 24.68% <0.00%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@chilagrow chilagrow marked this pull request as ready for review August 7, 2023 11:48
@chilagrow chilagrow requested a review from a team as a code owner August 7, 2023 11:48
@chilagrow chilagrow requested review from rumyantseva, a team and noisersup August 7, 2023 11:48
@chilagrow chilagrow enabled auto-merge (squash) August 7, 2023 11:49
@chilagrow chilagrow marked this pull request as draft August 7, 2023 13:21
auto-merge was automatically disabled August 7, 2023 13:21

Pull request was converted to draft

Copy link
Contributor

@rumyantseva rumyantseva left a 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.

internal/handlers/common/aggregations/expression.go Outdated Show resolved Hide resolved
internal/handlers/common/filter_iterator.go Outdated Show resolved Hide resolved
internal/handlers/common/filter_iterator.go Outdated Show resolved Hide resolved
@chilagrow chilagrow marked this pull request as ready for review August 8, 2023 04:41
@chilagrow chilagrow enabled auto-merge (squash) August 8, 2023 04:41
@chilagrow chilagrow requested review from rumyantseva and a team August 8, 2023 04:42
@chilagrow chilagrow changed the title Implement $expr query operator Implement $expr evaluation query operator Aug 8, 2023
@chilagrow chilagrow requested a review from rumyantseva August 8, 2023 09:49
Copy link
Member

@noisersup noisersup left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rumyantseva rumyantseva left a comment

Choose a reason for hiding this comment

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

LGTM!

@chilagrow chilagrow merged commit 9a77e92 into FerretDB:main Aug 9, 2023
@AlekSi AlekSi added this to the v1.8.0 milestone Aug 14, 2023
"go.mongodb.org/mongo-driver/bson"
)

func TestAggregateCompatMatchExpr(t *testing.T) {
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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 🤗

Copy link
Member

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.

@chilagrow chilagrow mentioned this pull request Aug 28, 2023
9 tasks
yonarw pushed a commit to yonarw/FerretDB that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/feature Some user-visible feature is not implemented yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support $expr evaluation query operator
4 participants