Paginated datastore API #1265
Paginated datastore API #1265
Conversation
github-actions
bot
added
area/api v0
jakedt
force-pushed
the
paginated-datastore
branch
3 times, most recently
from
aa6ec4c
to
8edefc7
Compare
There was a problem hiding this comment.
we probably want to add unit tests for both modes of TupleOrder and for After
There was a problem hiding this comment.
There are datastore tests for both modes of TupleOrder in the pagination.go test suite. Does that work, or are you thinking about something else?
internal/datastore/memdb/readonly.go
Outdated
| @@ -345,16 +345,99 @@ func filterFuncForFilters( | |||
| return !found | |||
| } | |||
|
|
|||
| tpl := &core.RelationTuple{ | |||
There was a problem hiding this comment.
I know this is memdb and all but I wonder if we can do avoid this allocation, specially if this method is to be applied over a potentially long list of tuples. Could we perhaps take the raw relationship?
There was a problem hiding this comment.
friendly ping ☝🏻 can we make this non-allocating?
jakedt
force-pushed
the
paginated-datastore
branch
3 times, most recently
from
7bdf590
to
f9be65f
Compare
jakedt
force-pushed
the
paginated-datastore
branch
from
2e9e0a7
to
f96b17b
Compare
| ResourceType: tc.objectType, | ||
| }, options.WithLimit(&testLimit), options.WithAfter(&core.RelationTuple{})) | ||
| require.ErrorIs(err, datastore.ErrCursorsWithoutSorting) | ||
|
|
There was a problem hiding this comment.
I think this could be a good place to also test the new paginatedIterator given it has no tests, to make sure it properly does resuming based on batchSize
There was a problem hiding this comment.
I think of the paginated iterator as a helper written from the perspective of the datastore consumer. As such I don't think tests for it belong here. Ideally it would test using mocks. Let me see how long it will take to write that.
internal/datastore/memdb/readonly.go
Outdated
| @@ -345,16 +345,99 @@ func filterFuncForFilters( | |||
| return !found | |||
| } | |||
|
|
|||
| tpl := &core.RelationTuple{ | |||
There was a problem hiding this comment.
friendly ping ☝🏻 can we make this non-allocating?
|
|
||
| // NewPaginatedIterator creates an implementation of the datastore.Iterator | ||
| // interface that internally paginates over datastore results. | ||
| func NewPaginatedIterator( |
There was a problem hiding this comment.
Does not have a test, but perhaps as I proposed in pkg/datastore/test/pagination.go, we could do integration test for the iterator there
| t.Run("TestOrdering", func(t *testing.T) { OrderingTest(t, tester) }) | ||
| t.Run("TestLimit", func(t *testing.T) { LimitTest(t, tester) }) | ||
| t.Run("TestOrderedLimit", func(t *testing.T) { OrderedLimitTest(t, tester) }) | ||
| t.Run("TestResume", func(t *testing.T) { ResumeTest(t, tester) }) |
There was a problem hiding this comment.
Might be better to be specific here: TestOrderedQueryRels, TestResumeQueryRels, etc
| @@ -77,18 +95,120 @@ func NewSchemaQueryFilterer(schema SchemaInformation, initialQuery sq.SelectBuil | |||
| } | |||
| } | |||
|
|
|||
| func (sqf SchemaQueryFilterer) TupleOrder(order options.SortOrder) SchemaQueryFilterer { | |||
There was a problem hiding this comment.
Can we add SQL tests for this (and below)
jakedt
force-pushed
the
paginated-datastore
branch
from
f96b17b
to
2488aca
Compare
jakedt
force-pushed
the
paginated-datastore
branch
from
7466b6f
to
7f1e5d6
Compare
No description provided.