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

Support $eq comparision operator #309

Merged
merged 11 commits into from
Apr 1, 2022
Merged

Conversation

ribaraka
Copy link
Contributor

@ribaraka ribaraka commented Feb 7, 2022

Refs #251.

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

Please sign the CLA, merge with main, fix CI issues

.gitignore Outdated Show resolved Hide resolved
internal/handlers/jsonb1/where.go Outdated Show resolved Hide resolved
internal/handlers/msg_comparison_test.go Outdated Show resolved Hide resolved
internal/handlers/msg_comparison_test.go Outdated Show resolved Hide resolved
internal/handlers/msg_comparison_test.go Outdated Show resolved Hide resolved
@ribaraka ribaraka marked this pull request as ready for review February 8, 2022 14:18
@ribaraka ribaraka force-pushed the issue-251-eq branch 2 times, most recently from 64d5d8a to 633408e Compare February 8, 2022 17:12
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #309 (67d8bbf) into main (8186b0a) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #309      +/-   ##
==========================================
+ Coverage   57.39%   57.65%   +0.25%     
==========================================
  Files         117      117              
  Lines        6014     6031      +17     
==========================================
+ Hits         3452     3477      +25     
+ Misses       2047     2039       -8     
  Partials      515      515              
Flag Coverage Δ
FerretDB 32.25% <18.51%> (-0.09%) ⬇️
MongoDB 2.70% <0.00%> (-0.01%) ⬇️
integration 32.29% <18.51%> (-0.09%) ⬇️
tests 51.19% <100.00%> (+0.27%) ⬆️

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

Impacted Files Coverage Δ
internal/handlers/common/compare.go 84.30% <100.00%> (+1.56%) ⬆️
internal/handlers/common/filter.go 84.90% <100.00%> (+1.62%) ⬆️
internal/util/testutil/equal.go 48.79% <0.00%> (+1.80%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8186b0a...67d8bbf. Read the comment docs.

internal/handlers/jsonb1/where.go Outdated Show resolved Hide resolved
internal/handlers/msg_comparison_test.go Outdated Show resolved Hide resolved
internal/handlers/msg_comparison_test.go Outdated Show resolved Hide resolved
@AlekSi
Copy link
Member

AlekSi commented Feb 11, 2022

Where is a PR to https://github.com/FerretDB/dance?

internal/handlers/handler_test.go Outdated Show resolved Hide resolved
internal/handlers/handler_test.go Outdated Show resolved Hide resolved
internal/handlers/jsonb1/where.go Outdated Show resolved Hide resolved
internal/handlers/jsonb1/where.go Outdated Show resolved Hide resolved
@AlekSi AlekSi added this to the v0.0.7 milestone Feb 17, 2022
@AlekSi AlekSi added code/feature Some user-visible feature is not implemented yet trust PRs that can access Actions secrets labels Feb 17, 2022
@ribaraka ribaraka requested a review from AlekSi February 20, 2022 10:33
@AlekSi AlekSi changed the title Support all comparison query operators [WIP] Support `$eq§ comparision operator Feb 22, 2022
@AlekSi AlekSi changed the title Support `$eq§ comparision operator Support $eq comparision operator Feb 22, 2022
Copy link
Member

@AlekSi AlekSi left a comment

Choose a reason for hiding this comment

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

Some of our dance tests fail with MongoDB, meaning that FerretDB and MongoDB behave differently. That's a problem that should be fixed.

@ribaraka
Copy link
Contributor Author

I removed the TODO regex from the $eq comparison operator. I have not find any information in the documentation and on the Internet that these two expressions were used.
I also tried to issue query in mongosh db.values.find( { value: {$eq :/foo/i } } ) to the values database manually and it returns only match stored regexes. I mean is there possibility who would store regex datatype in a document? Anyway current implementation FerretDB returns nothing.
What do you think we will do next?

@AlekSi
Copy link
Member

AlekSi commented Mar 31, 2022

I mean is there possibility who would store regex datatype in a document?

Yes.

Anyway current implementation FerretDB returns nothing.

We should add a dance test for that case and fix that difference in behavior between MongoDB and FerretDB.

internal/handlers/common/filter.go Outdated Show resolved Hide resolved
@ribaraka ribaraka force-pushed the issue-251-eq branch 2 times, most recently from bfeb590 to b0fd03b Compare March 31, 2022 15:32
internal/handlers/common/filter.go Outdated Show resolved Hide resolved
internal/handlers/common/filter.go Outdated Show resolved Hide resolved
@AlekSi AlekSi merged commit 3eda589 into FerretDB:main Apr 1, 2022
@AlekSi
Copy link
Member

AlekSi commented Apr 1, 2022

Thank you!

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 trust PRs that can access Actions secrets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants