-
Couldn't load subscription status.
- Fork 346
perf: Have read relationships use dynamic index forcing #2632
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (59.15%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## main #2632 +/- ##
===========================================
- Coverage 79.52% 59.15% -20.36%
===========================================
Files 454 412 -42
Lines 47138 43740 -3398
===========================================
- Hits 37481 25870 -11611
- Misses 6894 15369 +8475
+ Partials 2763 2501 -262 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Shall we rename this PR from feat: ... to perf: ... since it's not a new feature but rather a performance improvement?
| BySubject | ||
|
|
||
| // ChooseEfficient lets the datastore choose the most efficient order based on the query shape and available indexes. | ||
| ChooseEfficient |
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.
why give the caller a new option, when unsorted: lets the underlying datastore choose the order exists? or should we update unsorted to just say no order at all?
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.
Because sometimes the caller knows better and letting it choose is important in those instances
| ds, | ||
| dsFilter, | ||
| pageSize, | ||
| options.ByResource, |
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.
By reading https://buf.build/authzed/api/docs/main:authzed.api.v1#authzed.api.v1.PermissionsService.ReadRelationships it doesn't seem that we ever promised a particular return order. I'm curious why this code was the way it is with options.ByResource? was this an undocumented feature or just an accident?
can we update the API to be explicit that callers shouldn't expect any order?
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.
Its necessary for cursoring; we need some stable order in order to cursor
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.
ok; should we update the API docs to say something like "the results will be sorted but the order will depend on the filter passed"?
This should ensure that, for recognized query shapes, the best index and sort order is selected for ReadRels
ce76426 to
45bdbf6
Compare
This should ensure that, for recognized query shapes, the best index and sort order is selected for ReadRels
Testing
Unit, datatstore and steelthread tests