Support `_id` implicit filter for `ObjectID` in SQLite #3330

chilagrow · 2023-09-08T09:43:48Z

Description

Closes #2340.

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 · 2023-09-08T09:46:20Z

Codecov Report

Merging #3330 (f1af2db) into main (17aed11) will decrease coverage by 0.89%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3330      +/-   ##
==========================================
- Coverage   73.84%   72.95%   -0.89%     
==========================================
  Files         413      413              
  Lines       25034    25088      +54     
==========================================
- Hits        18486    18304     -182     
- Misses       5463     5676     +213     
- Partials     1085     1108      +23

Files Changed	Coverage
internal/backends/collection.go	`ø`
internal/backends/sqlite/collection.go	`100.00%`
internal/handlers/sqlite/msg_count.go	`100.00%`
internal/handlers/sqlite/msg_delete.go	`100.00%`
internal/handlers/sqlite/msg_distinct.go	`100.00%`
internal/handlers/sqlite/msg_explain.go	`100.00%`
internal/handlers/sqlite/msg_find.go	`100.00%`
internal/handlers/sqlite/msg_findandmodify.go	`100.00%`
internal/handlers/sqlite/msg_update.go	`100.00%`

Flag	Coverage Δ
hana-1	`?`
hana-2	`?`
hana-3	`?`
integration	`69.89% <100.00%> (-0.94%)`	⬇️
mongodb-1	`4.71% <0.00%> (-0.02%)`	⬇️
pg-1	`44.31% <0.00%> (-0.09%)`	⬇️
pg-2	`43.84% <0.00%> (-0.10%)`	⬇️
pg-3	`?`
sqlite-1	`42.90% <92.85%> (+0.12%)`	⬆️
sqlite-2	`42.00% <92.85%> (+0.10%)`	⬆️
sqlite-3	`42.35% <92.85%> (+0.21%)`	⬆️
unit	`23.23% <0.00%> (-0.06%)`	⬇️

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

internal/backends/collection.go

internal/backends/sqlite/collection.go

chilagrow · 2023-09-11T04:40:52Z

Implicit filter with objectID is pushed down

test> db.chi.find({_id: ObjectId("64fe76829d57c1b7ded6b5de")})
[ { _id: ObjectId("64fe76829d57c1b7ded6b5de"), v: 1 } ]

2023-09-11T17:25:51.378+0900    DEBUG   sqlite.test     fsql/db.go:77   >>> SELECT _ferretdb_sjson FROM "chi_f5858a65" WHERE _ferretdb_sjson->'$._id' = ?       {"args": ["\"64fe76829d57c1b7ded6b5de\""]}
2023-09-11T17:25:51.379+0900    DEBUG   sqlite.test     fsql/db.go:82   <<< SELECT _ferretdb_sjson FROM "chi_f5858a65" WHERE _ferretdb_sjson->'$._id' = ?       {"args": ["\"64fe76829d57c1b7ded6b5de\""], "
time": "1.001714ms"}

Other types are not pushed down

test> db.chi.find({_id: "stringID"})
[ { _id: 'stringID', v: 2 } ]

2023-09-11T17:26:16.842+0900    DEBUG   sqlite.test     fsql/db.go:77   >>> SELECT _ferretdb_sjson FROM "chi_f5858a65"  {"args": null}
2023-09-11T17:26:16.842+0900    DEBUG   sqlite.test     fsql/db.go:82   <<< SELECT _ferretdb_sjson FROM "chi_f5858a65"  {"args": null, "time": "725.445µs"}

$eq filter is not pushed down

test> db.chi.find({_id: {$eq: ObjectId("64fe76829d57c1b7ded6b5de")}})
[ { _id: ObjectId("64fe76829d57c1b7ded6b5de"), v: 1 } ]

2023-09-11T17:26:53.465+0900    DEBUG   sqlite.test     fsql/db.go:77   >>> SELECT _ferretdb_sjson FROM "chi_f5858a65"  {"args": null}                                                                      
2023-09-11T17:26:53.466+0900    DEBUG   sqlite.test     fsql/db.go:82   <<< SELECT _ferretdb_sjson FROM "chi_f5858a65"  {"args": null, "time": "996.152µs"}

Taskfile.yml

internal/backends/sqlite/collection.go

internal/backends/collection.go

integration/query_compat_test.go

internal/handlers/sqlite/msg_update.go

internal/handlers/sqlite/msg_findandmodify.go

rumyantseva

Looks good to me overall, asked a few questions/comments.

I know we want to release it ASAP, so please feel free to ignore the comments that are not important.

Taskfile.yml

internal/backends/sqlite/collection.go

integration/query_compat_test.go

rumyantseva · 2023-09-11T06:11:02Z

@chilagrow from your examples this one looks interesting:

2023-09-11T13:05:25.974+0900    DEBUG   sqlite.test     fsql/db.go:82   <<< SELECT _ferretdb_sjson FROM "chi_f5858a65" WHERE _ferretdb_sjson->'$._id' = ?       {"args": ["\"64fe76829d57c1b7ded6b5de\""], "

so, it is searched as a string in quotes? it looks a bit odd. is it how it's stored in the DB? if we apply this filter manually in the sqlite console, are we able to find the entry?

chilagrow · 2023-09-11T06:48:18Z

@chilagrow from your examples this one looks interesting:
2023-09-11T13:05:25.974+0900    DEBUG   sqlite.test     fsql/db.go:82   <<< SELECT _ferretdb_sjson FROM "chi_f5858a65" WHERE _ferretdb_sjson->'$._id' = ?       {"args": ["\"64fe76829d57c1b7ded6b5de\""], "
so, it is searched as a string in quotes? it looks a bit odd. is it how it's stored in the DB? if we apply this filter manually in the sqlite console, are we able to find the entry?

@rumyantseva When we use operator -> we need quoted string to match. See https://www.sqlite.org/json1.html#jptr. If we use ->> operator, we won't need quoted string as far as I can tell from SQLite doc.

rumyantseva

LGTM!

Special thanks for answering my questions :)

hack up sqlite pushdown for objectID for implict eq

ebf337b

chilagrow added the code/feature Some user-visible feature is not implemented yet label Sep 8, 2023

chilagrow self-assigned this Sep 8, 2023

Merge branch 'main' into issue-2340-pushdown-eq-objectid

f6206c8

AlekSi reviewed Sep 10, 2023

View reviewed changes

internal/backends/collection.go Outdated Show resolved Hide resolved

internal/backends/sqlite/collection.go Outdated Show resolved Hide resolved

internal/backends/sqlite/collection.go Outdated Show resolved Hide resolved

internal/backends/sqlite/collection.go Outdated Show resolved Hide resolved

AlekSi and others added 5 commits September 11, 2023 00:47

Merge branch 'main' into issue-2340-pushdown-eq-objectid

5e6fe76

remove unnecessary code

51c434d

fix

cff97c6

filter is string not byte

0069c6c

test runs with query pushdown

cf6b9f9

chilagrow added 4 commits September 11, 2023 13:54

integration test enables filter pushdown

ffa57eb

check sqlite pushdown for tests

712fdc7

fix test

2ab5cb0

update and findAndModify uses pushdown

f35326b

chilagrow commented Sep 11, 2023

View reviewed changes

chilagrow marked this pull request as ready for review September 11, 2023 05:47

chilagrow requested a review from a team as a code owner September 11, 2023 05:47

chilagrow requested review from AlekSi, rumyantseva, a team and noisersup September 11, 2023 05:47

chilagrow added this to the Next milestone Sep 11, 2023

chilagrow enabled auto-merge (squash) September 11, 2023 05:48

rumyantseva reviewed Sep 11, 2023

View reviewed changes

Taskfile.yml Show resolved Hide resolved

internal/backends/sqlite/collection.go Outdated Show resolved Hide resolved

internal/backends/sqlite/collection.go Outdated Show resolved Hide resolved

integration/query_compat_test.go Show resolved Hide resolved

add test, fix sql param handling

cf247c0

chilagrow requested a review from rumyantseva September 11, 2023 06:49

rumyantseva previously approved these changes Sep 11, 2023

View reviewed changes

use pushdown in count delete and distinct

998436f

chilagrow dismissed rumyantseva’s stale review via 998436f September 11, 2023 07:43

chilagrow requested a review from rumyantseva September 11, 2023 09:24

Merge branch 'main' into issue-2340-pushdown-eq-objectid

7d3cc48

AlekSi modified the milestones: Next, v1.10.0 Sep 11, 2023

Fix condition, add TODOs

f1af2db

rumyantseva approved these changes Sep 11, 2023

View reviewed changes

AlekSi approved these changes Sep 11, 2023

View reviewed changes

chilagrow merged commit ee5e0e8 into FerretDB:main Sep 11, 2023

b1ron mentioned this pull request Oct 16, 2023

Investigate workloadc performance problem FerretDB/dance#625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `_id` implicit filter for `ObjectID` in SQLite #3330

Support `_id` implicit filter for `ObjectID` in SQLite #3330

chilagrow commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

chilagrow commented Sep 11, 2023 •

edited

Loading

rumyantseva left a comment •

edited

Loading

rumyantseva commented Sep 11, 2023

chilagrow commented Sep 11, 2023 •

edited

Loading

rumyantseva left a comment

Support _id implicit filter for ObjectID in SQLite #3330

Support _id implicit filter for ObjectID in SQLite #3330

Conversation

chilagrow commented Sep 8, 2023 • edited Loading

Description

Readiness checklist

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

chilagrow commented Sep 11, 2023 • edited Loading

rumyantseva left a comment • edited Loading

Choose a reason for hiding this comment

rumyantseva commented Sep 11, 2023

chilagrow commented Sep 11, 2023 • edited Loading

rumyantseva left a comment

Choose a reason for hiding this comment

Support `_id` implicit filter for `ObjectID` in SQLite #3330

Support `_id` implicit filter for `ObjectID` in SQLite #3330

chilagrow commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

chilagrow commented Sep 11, 2023 •

edited

Loading

rumyantseva left a comment •

edited

Loading

chilagrow commented Sep 11, 2023 •

edited

Loading