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.

Sign up for GitHub

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

Jump to bottom

Allow query to be type null in distinct command #2658

Merged
merged 16 commits into from
Jun 1, 2023
Merged

Allow query to be type null in distinct command #2658

merged 16 commits into from
Jun 1, 2023

Conversation

b1ron
Copy link
Contributor

@b1ron b1ron commented May 18, 2023 •
edited
Loading

Description

Closes #2236.

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • 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.

@b1ron b1ron self-assigned this May 18, 2023
@b1ron b1ron added the code/bug Some user-visible feature works incorrectly label May 18, 2023
@b1ron b1ron changed the title Allow null type in distinct command Allow query to be type null in distinct command May 18, 2023
@codecov
Copy link

codecov bot commented May 18, 2023 •
edited
Loading

Codecov Report

Merging #2658 (18e4082) into main (ad0e824) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2658      +/-   ##
==========================================
+ Coverage   63.04%   63.06%   +0.02%     
==========================================
  Files         441      441              
  Lines       22619    22633      +14     
==========================================
+ Hits        14260    14274      +14     
  Misses       7434     7434              
  Partials      925      925
Impacted Files Coverage Δ
internal/handlers/common/distinct.go 90.47% <100.00%> (+2.72%) ⬆️
Flag Coverage Δ
integration 56.51% <100.00%> (+0.02%) ⬆️
mongodb 4.96% <0.00%> (-0.01%) ⬇️
pg 56.43% <100.00%> (+0.02%) ⬆️
unit 24.62% <0.00%> (-0.04%) ⬇️

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

@b1ron b1ron marked this pull request as ready for review May 18, 2023 08:05
@b1ron b1ron requested a review from a team as a code owner May 18, 2023 08:05
@b1ron b1ron requested review from AlekSi, chilagrow, a team, w84thesun, rumyantseva and noisersup May 18, 2023 08:05
@b1ron b1ron enabled auto-merge (squash) May 18, 2023 08:05
chilagrow
chilagrow reviewed May 18, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Nice catch. Let's add more tests!

internal/handlers/common/distinct.go Outdated Show resolved Hide resolved
internal/handlers/pg/msg_distinct.go Outdated Show resolved Hide resolved
@AlekSi AlekSi added the do not merge PRs that should not be merged label May 18, 2023
@AlekSi AlekSi added this to the Next milestone May 18, 2023
noisersup
noisersup reviewed May 18, 2023
View reviewed changes
internal/handlers/tigris/msg_distinct.go Outdated Show resolved Hide resolved
integration/distinct_compat_test.go Outdated Show resolved Hide resolved
integration/distinct_test.go Outdated Show resolved Hide resolved
chilagrow
chilagrow requested changes May 19, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

I missed an important logic first round, please take a look. 🤗

internal/handlers/common/distinct.go Show resolved Hide resolved
internal/handlers/pg/msg_distinct.go Outdated Show resolved Hide resolved
internal/handlers/common/distinct.go Outdated Show resolved Hide resolved
@AlekSi AlekSi removed the do not merge PRs that should not be merged label May 19, 2023
rumyantseva
rumyantseva reviewed May 19, 2023
View reviewed changes
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.

Hmm, this solution is not what we discussed. Do you think it's possible to try our original approach? With checking if null is possible for other optional params too and changing the common logic, not only distinct logic.

@AlekSi AlekSi marked this pull request as draft May 19, 2023 08:47
auto-merge was automatically disabled May 19, 2023 08:47

Pull request was converted to draft

@AlekSi AlekSi modified the milestones: v1.2.0, Next May 22, 2023
@mergify
Copy link
Contributor

mergify bot commented May 29, 2023

@b1ron this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label May 29, 2023
Dmitry added 2 commits May 30, 2023 18:56
WIP
03c1b55
Merge branch 'main' into pr/2658
5b3e49f 
# Conflicts:
#	integration/distinct_test.go
#	integration/findandmodify_test.go
#	integration/insert_command_compat_test.go
#	internal/handlers/common/params.go
@mergify mergify bot removed the conflict PRs that have merge conflicts label May 31, 2023
@w84thesun
Copy link
Contributor

As discussed on DS, I suggest merging this PR with the solution for the distinct command only and making any decisions on how to process that kind of parameter in #2704.

@w84thesun w84thesun marked this pull request as ready for review May 31, 2023 09:12
@w84thesun w84thesun enabled auto-merge (squash) May 31, 2023 09:12
chilagrow
chilagrow reviewed May 31, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

Just small comment 🤗

internal/handlers/common/distinct.go Show resolved Hide resolved
internal/handlers/common/distinct.go Show resolved Hide resolved
rumyantseva
rumyantseva reviewed May 31, 2023
View reviewed changes
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 agree with Chi's comments, I don't have new questions.

rumyantseva
rumyantseva approved these changes May 31, 2023
View reviewed changes
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.

Looks reasonable to me as a temporary solution!

noisersup
noisersup approved these changes May 31, 2023
View reviewed changes
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.

Makes sense for now!

@w84thesun w84thesun merged commit 4a69d97 into FerretDB:main Jun 1, 2023
@b1ron b1ron mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noisersup noisersup noisersup approved these changes

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

@w84thesun w84thesun Awaiting requested review from w84thesun w84thesun was automatically assigned from FerretDB/core

@AlekSi AlekSi Awaiting requested review from AlekSi

Assignees

@b1ron b1ron

Labels
code/bug Some user-visible feature works incorrectly
Projects
Current Engineering
Archived in project
Milestone
v1.3.0
Development

Successfully merging this pull request may close these issues.

Allow query param is be of type null in distinct command
6 participants
@b1ron @w84thesun @AlekSi @rumyantseva @chilagrow @noisersup