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 filter pushdown for SQLite #3337

Closed
wants to merge 86 commits into from

Conversation

noisersup
Copy link
Member

@noisersup noisersup commented Sep 11, 2023

Description

Closes #3235.

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 Sep 11, 2023

Codecov Report

Merging #3337 (db49f3e) into main (3572f9b) will decrease coverage by 5.18%.
Report is 1 commits behind head on main.
The diff coverage is 86.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3337      +/-   ##
==========================================
- Coverage   74.44%   69.26%   -5.18%     
==========================================
  Files         422      417       -5     
  Lines       26375    26163     -212     
==========================================
- Hits        19635    18122    -1513     
- Misses       5617     6925    +1308     
+ Partials     1123     1116       -7     
Files Coverage Δ
internal/backends/sqlite/collection.go 74.82% <72.72%> (-2.56%) ⬇️
internal/backends/sqlite/pushdown.go 90.00% <90.00%> (ø)

... and 103 files with indirect coverage changes

Flag Coverage Δ
filter-false ?
filter-true 69.26% <86.88%> (-0.49%) ⬇️
hana-1 ?
integration 69.26% <86.88%> (-0.59%) ⬇️
mongodb-1 4.61% <0.00%> (-0.02%) ⬇️
pg-1 40.45% <0.00%> (-0.72%) ⬇️
pg-2 43.16% <0.00%> (-0.55%) ⬇️
pg-3 42.73% <0.00%> (-0.80%) ⬇️
postgresql-1 ∅ <ø> (∅)
postgresql-2 ?
postgresql-3 ?
sort-false 69.26% <86.88%> (-0.16%) ⬇️
sort-true ?
sqlite-1 39.75% <54.09%> (-0.04%) ⬇️
sqlite-2 42.62% <86.88%> (+0.14%) ⬆️
sqlite-3 42.48% <64.75%> (+0.04%) ⬆️
unit ?

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

chilagrow
chilagrow previously approved these changes Sep 25, 2023
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.

Great work 👍

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.

Almost there

Comment on lines 28 to 30
noPushdown resultPushdown = 1 << iota // 0000 0000
pgPushdown // 0000 0001
sqlitePushdown // 0000 0010
Copy link
Member

Choose a reason for hiding this comment

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

Those comments are not actually correct. It is better to check them in the init function

Comment on lines 43 to 48
case setup.IsSQLite(t):
return res&sqlitePushdown == sqlitePushdown
case setup.IsPostgres(t):
return res&pgPushdown == pgPushdown
case setup.IsMongoDB(t):
return false
Copy link
Member

Choose a reason for hiding this comment

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

Let's use consistent order: PostgreSQL, SQLite, other backends, MongoDB

// IsPostgres returns true if the current test is running for pg handler.
//
// This function should not be used lightly.
func IsPostgres(tb testtb.TB) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Move it up to preserve the order

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@noisersup this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Sep 27, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label Sep 27, 2023
@noisersup noisersup requested a review from AlekSi September 27, 2023 13:06
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.

As discussed on DS, let's split that PR into two: one for tests changes (that will be need for the new PG backend), one for SQLite pushdown

@mergify
Copy link
Contributor

mergify bot commented Sep 28, 2023

@noisersup this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Sep 28, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label Sep 28, 2023
@noisersup noisersup requested a review from AlekSi September 29, 2023 04:10
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.

Is this one in pause right?

@AlekSi AlekSi marked this pull request as draft October 4, 2023 05:00
auto-merge was automatically disabled October 4, 2023 05:00

Pull request was converted to draft

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2023

@noisersup this pull request has merge conflicts.

@mergify mergify bot added the conflict PRs that have merge conflicts label Oct 6, 2023
@AlekSi AlekSi removed this from the v1.12.0 milestone Oct 9, 2023
@AlekSi AlekSi closed this Nov 14, 2023
@mergify mergify bot removed the conflict PRs that have merge conflicts label Nov 14, 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.

Implement filter pushdown for SQLite
3 participants