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

Implement limit pushdown for PostgreSQL #3580

Merged
merged 17 commits into from
Oct 18, 2023
Merged

Implement limit pushdown for PostgreSQL #3580

merged 17 commits into from
Oct 18, 2023

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented Oct 16, 2023 •
edited
Loading

Description

Closes #3416.

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), Milestone (Next), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@codecov
Copy link

codecov bot commented Oct 16, 2023 •
edited
Loading

Codecov Report

Merging #3580 (7f0ad57) into main (0f08f61) will decrease coverage by 0.11%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3580      +/-   ##
==========================================
- Coverage   61.65%   61.54%   -0.11%     
==========================================
  Files         426      426              
  Lines       27480    27528      +48     
==========================================
+ Hits        16942    16943       +1     
- Misses       9532     9573      +41     
- Partials     1006     1012       +6
Files Coverage Δ
internal/backends/collection.go 100.00% <ø> (ø)
internal/backends/postgresql/collection.go 64.47% <100.00%> (+0.79%) ⬆️
internal/handlers/common/explain.go 75.00% <100.00%> (ø)
internal/handlers/common/params.go 94.78% <100.00%> (+0.20%) ⬆️
internal/handlers/sqlite/msg_explain.go 57.83% <100.00%> (+1.58%) ⬆️
internal/handlers/sqlite/msg_find.go 72.72% <100.00%> (+0.63%) ⬆️
internal/backends/sqlite/collection.go 79.88% <85.00%> (+1.04%) ⬆️

... and 8 files with indirect coverage changes

Flag Coverage Δ
filter-true 57.93% <94.23%> (-0.05%) ⬇️
hana-1 ?
integration 57.93% <94.23%> (-0.05%) ⬇️
mongodb-1 4.49% <0.00%> (-0.01%) ⬇️
postgresql-1 39.05% <34.61%> (-1.96%) ⬇️
postgresql-2 41.72% <44.23%> (+0.04%) ⬆️
postgresql-3 41.89% <44.23%> (+1.96%) ⬆️
sort-false 57.93% <94.23%> (-0.05%) ⬇️
sqlite-1 39.43% <50.00%> (-1.53%) ⬇️
sqlite-2 41.31% <59.61%> (-0.54%) ⬇️
sqlite-3 42.27% <59.61%> (+2.33%) ⬆️
unit 22.49% <0.00%> (-0.11%) ⬇️

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

@noisersup noisersup added the code/feature Some user-visible feature is not implemented yet label Oct 16, 2023
@noisersup noisersup added this to the Next milestone Oct 16, 2023
@noisersup noisersup marked this pull request as ready for review October 16, 2023 04:12
@noisersup noisersup requested review from AlekSi and a team as code owners October 16, 2023 04:12
@noisersup noisersup requested review from rumyantseva, a team and chilagrow October 16, 2023 04:12
@noisersup noisersup enabled auto-merge (squash) October 16, 2023 04:31
@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2023

@noisersup this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Oct 17, 2023
AlekSi
AlekSi reviewed Oct 17, 2023
View reviewed changes
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.

image

@noisersup
Copy link
Member Author

@AlekSi Not only conflicts, there are failing tests and I'm tweaking that

@mergify mergify bot removed the conflict PRs that have merge conflicts label Oct 17, 2023
@noisersup noisersup requested review from AlekSi and chilagrow October 17, 2023 14:20
@AlekSi AlekSi requested review from a team October 17, 2023 14:21
AlekSi
AlekSi reviewed Oct 17, 2023
View reviewed changes
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.

#3580 (review)

(that seems to be a new GitHub bug – reviews without the final review comment are not marked as reviews)

@noisersup noisersup requested a review from AlekSi October 17, 2023 23:28
chilagrow
chilagrow reviewed Oct 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.

There are failing tests

internal/handlers/common/params.go Show resolved Hide resolved
rumyantseva
rumyantseva previously approved these changes Oct 18, 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.

The code looks good to me, but I agree with Chi: it would be nice to add a test case to cover the situation where you found a bug.

@noisersup noisersup mentioned this pull request Oct 18, 2023
Open
@noisersup noisersup requested a review from rumyantseva October 18, 2023 12:00
@noisersup
Copy link
Member Author

CI is failing due to coveralls, the tests passed

@AlekSi AlekSi requested review from a team October 18, 2023 12:01
@noisersup noisersup changed the title Implement limit pushdown for PostgreSQL Implement limit pushdown for PostgreSQL Oct 18, 2023
rumyantseva
rumyantseva approved these changes Oct 18, 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.

LGTM

@AlekSi AlekSi disabled auto-merge October 18, 2023 16:02
@AlekSi AlekSi merged commit ae0bca1 into FerretDB:main Oct 18, 2023
32 of 43 checks passed
@noisersup noisersup deleted the postgresql-limit-pushdown-3416 branch October 18, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rumyantseva rumyantseva rumyantseva approved these changes

@AlekSi AlekSi AlekSi approved these changes

@chilagrow chilagrow Awaiting requested review from chilagrow

Assignees

@noisersup noisersup

Labels
code/feature Some user-visible feature is not implemented yet
Projects
Current Engineering
Archived in project
Milestone
v1.13.0
Development

Successfully merging this pull request may close these issues.

Implement limit pushdown
4 participants
@noisersup @AlekSi @rumyantseva @chilagrow