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

Add filter pushdown for _id: <string> for SQLite #3599

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Add filter pushdown for _id: <string> for SQLite #3599

merged 3 commits into from
Oct 18, 2023

Conversation

AlekSi
Copy link
Member

@AlekSi AlekSi commented Oct 17, 2023 •
edited
Loading

Description

It is an ad-hoc filter for an important use-case that covers only the _id field because it can't be an array.

Refs #3235.

Closes FerretDB/dance#625.

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.

@AlekSi AlekSi self-assigned this Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023 •
edited
Loading

Codecov Report

Merging #3599 (e78ceab) into main (d8ac329) will decrease coverage by 0.55%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3599      +/-   ##
==========================================
- Coverage   61.80%   61.25%   -0.55%     
==========================================
  Files         426      426              
  Lines       27478    27476       -2     
==========================================
- Hits        16982    16830     -152     
- Misses       9501     9631     +130     
- Partials      995     1015      +20
Files Coverage Δ
integration/pushdown.go 61.11% <ø> (-16.67%) ⬇️
internal/backends/sqlite/collection.go 72.17% <100.00%> (-2.47%) ⬇️

... and 18 files with indirect coverage changes

Flag Coverage Δ
filter-false ?
filter-true 57.76% <100.00%> (-0.47%) ⬇️
hana-1 3.03% <0.00%> (+<0.01%) ⬆️
integration 57.76% <100.00%> (-0.49%) ⬇️
mongodb-1 4.50% <0.00%> (+<0.01%) ⬆️
postgresql-1 41.12% <0.00%> (-0.15%) ⬇️
postgresql-2 41.70% <0.00%> (-0.11%) ⬇️
postgresql-3 39.95% <0.00%> (-0.16%) ⬇️
sort-false 57.76% <100.00%> (-0.34%) ⬇️
sort-true ?
sqlite-1 ?
sqlite-2 41.87% <100.00%> (-0.10%) ⬇️
sqlite-3 39.99% <100.00%> (-0.15%) ⬇️
unit 22.39% <0.00%> (-0.08%) ⬇️

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

@AlekSi AlekSi added this to the Next milestone Oct 17, 2023
@AlekSi AlekSi added code/enhancement Some user-visible feature could work better trust PRs that can access Actions secrets packages PRs that should build packages labels Oct 17, 2023
@AlekSi AlekSi marked this pull request as ready for review October 17, 2023 11:53
@AlekSi AlekSi requested a review from a team as a code owner October 17, 2023 11:53
@AlekSi AlekSi enabled auto-merge (squash) October 17, 2023 11:53
@AlekSi AlekSi requested review from a team, noisersup and b1ron October 17, 2023 11:54
@AlekSi
Copy link
Member Author

AlekSi commented Oct 17, 2023

@b1ron please test that one

@b1ron
Copy link
Contributor

b1ron commented Oct 17, 2023

I am seeing an increase in performance by over two orders of magnitude. workloadc now completes in around 27s whereas before it just timed out.

No string pushdown took 1298.4s to read 371 records, it now takes 10s.

@b1ron b1ron mentioned this pull request Oct 17, 2023
Closed
rumyantseva
rumyantseva approved these changes Oct 17, 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.

:shipit:

chilagrow
chilagrow approved these changes 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.

🚀

@AlekSi AlekSi merged commit 65f5d13 into FerretDB:main Oct 18, 2023
25 of 26 checks passed
@AlekSi AlekSi deleted the simple-sqlite-pushdown branch October 18, 2023 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@rumyantseva rumyantseva rumyantseva approved these changes

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

@b1ron b1ron Awaiting requested review from b1ron

Assignees

@AlekSi AlekSi

Labels
code/enhancement Some user-visible feature could work better packages PRs that should build packages trust PRs that can access Actions secrets
Projects
Current Engineering
Archived in project
Milestone
v1.13.0
Development

Successfully merging this pull request may close these issues.

Investigate workloadc performance problem
4 participants
@AlekSi @b1ron @rumyantseva @chilagrow