-
Notifications
You must be signed in to change notification settings - Fork 409
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
Pushdown simplest sorting for find
command
#2506
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2506 +/- ##
==========================================
+ Coverage 64.34% 64.35% +0.01%
==========================================
Files 403 403
Lines 19956 20029 +73
==========================================
+ Hits 12840 12890 +50
- Misses 6187 6204 +17
- Partials 929 935 +6
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
@noisersup this pull request has merge conflicts. |
ORDER BY
find
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea and implementation looks good to me. To unblock you for aggregation implementation, I approve this PR.
To add in the second PR (apart from aggregation):
- a (unit) test for
iter, queryRes, err := QueryDocuments(ctxGet, tx, qp)
thatqueryRes
is returned correctly - check code cov if branches for sort pushdown are covered
(Please fix the failing tests) |
It seems that integration is not panicing right now, I've also added unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, looks good to me.
One tiny suggestion - as it costs nothing to include the info about sort pushdown to explain, let's add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
I tried it with and without the flag, works as expected!
test> db.foo.find().sort("hello").explain()
{
queryPlanner: {
Plan: {
'Node Type': 'Sort',
'Parallel Aware': false,
'Startup Cost': 97.78,
'Plan Width': 64,
Plans: [
{
'Parent Relationship': 'Outer',
'Parallel Aware': false,
'Async Capable': false,
Schema: 'test',
Alias: 'foo_a9f37ed7',
Output: [ '_jsonb', "(_jsonb -> 'hello'::text)" ],
'Node Type': 'Seq Scan',
'Startup Cost': 0,
'Total Cost': 27,
'Plan Rows': 1360,
'Plan Width': 64,
'Relation Name': 'foo_a9f37ed7'
}
],
'Async Capable': false,
'Total Cost': 101.18,
'Plan Rows': 1360,
Output: [ '_jsonb', "((_jsonb -> 'hello'::text))" ],
'Sort Key': [ "((foo_a9f37ed7._jsonb -> 'hello'::text))" ]
}
},
explainVersion: '1',
command: { find: 'foo', filter: {}, sort: { hello: 1 }, '$db': 'test' },
pushdown: false,
sortingPushdown: true,
serverInfo: {
host: 'ferretdb',
version: '6.0.42',
gitVersion: 'bc504ad73fbe30deb7096cc82d76f6ba8370cbaa',
ferretdbVersion: 'v1.0.0-95-gbc504ad7-dirty'
},
ok: 1
}
Description
Closes #2438.
The pushdown for the
find
command. Theaggregation
will come soon in other PR, just need to clarify some concerns.The PR also changes explain command to use internal data to specify if pushdown occured, rather than using Index cond/filter fields from postgres.
Readiness checklist
task all
, and it passed.@FerretDB/core
), Labels, Project and project's Sprint fields.