Skip to content
Closed
Changes from 1 commit
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
ff21c99
Impelement get_homogeneous_pages.
mattalbr Jul 11, 2023
eff5d52
Check in unit test for multi-page lookup.
mattalbr Jul 13, 2023
d37132b
Fix missing symbols.
mattalbr Jul 13, 2023
df7466c
Fix flake8 test_paging.py
mattalbr Jul 13, 2023
4843e68
Export get_homogeneous_pages.
mattalbr Jul 13, 2023
9985e4e
Actually import get_homogeneous_pages.
mattalbr Jul 13, 2023
bd486e1
Add PageRequest.
mattalbr Jul 13, 2023
e534243
Fix bad symbol.
mattalbr Jul 13, 2023
5e6fd6a
Extend gathered.
mattalbr Jul 13, 2023
f5075c3
Use Union instead of |.
mattalbr Jul 13, 2023
9aae97f
Use Tuple instead of tuple.
mattalbr Jul 13, 2023
cbefa30
Use Book instead of Animal.
mattalbr Jul 13, 2023
bb86b7e
More python 3.7 and fix wrong symbol name.
mattalbr Jul 13, 2023
cdd0fc2
Improve tests.
mattalbr Jul 13, 2023
2af1ca2
Remove unused db session.
mattalbr Jul 13, 2023
76c2939
Exclude sqlite.
mattalbr Jul 13, 2023
6867d83
s/dburl/no_sqlite_url
mattalbr Jul 13, 2023
19ff009
s/url/dburl
mattalbr Jul 13, 2023
abdef8a
Fix for loop variable.
mattalbr Jul 13, 2023
a5514cf
Per page should be at least 1.
mattalbr Jul 14, 2023
b7a34f5
Fix UNION ALL ordering which isn't guaranteed.
mattalbr Jul 14, 2023
2f4f4bd
NamedTuple is immutable.
mattalbr Jul 14, 2023
3909554
Trailing whitespace.
mattalbr Jul 14, 2023
3393641
Use uo instead of element to get ordering right for ROW_NUMBER.
mattalbr Jul 14, 2023
f985063
Use deque for extendleft functionality.
mattalbr Jul 14, 2023
e5feac2
Debugging info.
mattalbr Jul 14, 2023
46248d2
Testing.
mattalbr Jul 14, 2023
c5a606e
Add before preparing paging.
mattalbr Jul 14, 2023
3ccdd3e
author_id is nullable which doesn't work with paging.
mattalbr Jul 14, 2023
0e7cc0b
Actually end the test on success.
mattalbr Jul 14, 2023
1378936
Add a test for fetching columns.
mattalbr Jul 14, 2023
b610c69
Make test homogeneous.
mattalbr Jul 14, 2023
d46719f
Move page_identifier inside prepare_paging.
mattalbr Jul 14, 2023
bdac930
Add select_homogeneous_pages and refactor tests.
mattalbr Jul 14, 2023
1127c0f
flake8 fixes.
mattalbr Jul 14, 2023
24943ea
More flake8.
mattalbr Jul 14, 2023
5bb4c5d
Convert deque to list.
mattalbr Jul 14, 2023
29985c4
Test select_homogeneous_pages.
mattalbr Jul 14, 2023
87a6a08
Flake 8 violations.
mattalbr Jul 14, 2023
993e45f
Fix default.
mattalbr Jul 14, 2023
23ec6da
non-default before default.
mattalbr Jul 14, 2023
aff47d8
Add page_identifier
mattalbr Jul 14, 2023
dfee9dd
filter -> where.
mattalbr Jul 14, 2023
403a397
Add debugging.
mattalbr Jul 14, 2023
ead71a3
Fix select statements.
mattalbr Jul 14, 2023
685fca4
Go back to Book orm.
mattalbr Jul 14, 2023
ccb3e91
add a print statement.
mattalbr Jul 18, 2023
4f62fba
Start with the simplest test first.
mattalbr Jul 18, 2023
43694f0
flake8
mattalbr Jul 18, 2023
34c41b2
Print statement caused exception.
mattalbr Jul 18, 2023
db47749
Don't double order single selects.
mattalbr Jul 18, 2023
90e9ae7
Fix test_core..._empty_queries.
mattalbr Jul 18, 2023
8ecb334
flake8.
mattalbr Jul 18, 2023
6c33a0f
One more test.
mattalbr Jul 18, 2023
e230d1a
Try using sub-select for key generation.
mattalbr Jul 18, 2023
0917ceb
See if individualized selected works for select(Book)
mattalbr Jul 18, 2023
719440c
Try a different way of forming the union.
mattalbr Jul 18, 2023
e192516
flake8
mattalbr Jul 18, 2023
6b596a5
Backtrack on testing, plus add a more fundamental test.
mattalbr Jul 18, 2023
806ef9d
Add a test.
mattalbr Jul 18, 2023
fa5fd49
Only execute one subselect as necessary.
mattalbr Jul 18, 2023
1b9133b
flake8
mattalbr Jul 18, 2023
1909b8e
Clean up single query select.
mattalbr Jul 18, 2023
1af24f8
Uncomment test to send Anthony error.
mattalbr Jul 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions sqlakeyset/paging.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from __future__ import annotations

Copy link
Collaborator

Choose a reason for hiding this comment

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

CI fails at flake8 with a bunch of undefined names - looks like you've forgotten some imports?

from functools import partial
from dataclasses import dataclass
from typing import (
Any,
List,
Expand Down Expand Up @@ -438,3 +439,87 @@ def get_page(
place, backwards = process_args(after, before, page)

return orm_get_page(query, per_page, place, backwards)


# Do we need to support python 3.6?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, we've dropped support for 3.6 already and I'm happy to drop support for 3.7 if it gives us any friction at all. :)

Copy link
Author

Choose a reason for hiding this comment

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

Love it! dataclass is such a nice improvement from NamedTuple.

@dataclass
class PageRequest(Generic[_TP]):
"""See ``get_page()`` documentation for parameter explanations."""
query: Query[_TP]
per_page: int = PER_PAGE_DEFAULT
after: OptionalKeyset = None
before: OptionalKeyset = None
page: Optional[Union[MarkerLike, str]] = None


def get_homogeneous_page(queries: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I definitely think it should be get_homogeneous_pages.

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, typo!

"""Get multiple pages of results for homogeneous legacy ORM queries.

This only involves a single round trip to the database. To do that, under the
hood it generates a UNION ALL. That means each query must select exactly the
same columns. They may have different filters or ordering, but must result in
selecting the same columns with the same names.

Resulting pages are returned in the same order as the original page requests.
"""
if not queries:
return []

paging_queries = [_prepare_homogeneous_page(query, i) for i, query in enumerate(queries)]

query = paging_queries[0].query.query
query = query.union_all(*[q.query.query for q in paging_queries[1:]])

results = query.all()

# We need to make sure there's an entry for every page in case some return
# empty.
page_to_rows = {i: list() for i in range(len(queries))}
for row in results:
page_to_rows[row._page_identifier].append(row)

pages = []
for i in range(len(queries)):
rows = page_to_rows[i]
pages.append(paging_queries[i].page_from_rows(rows))
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing a return pages?

Copy link
Author

Choose a reason for hiding this comment

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

Done! Sorry about the lack of polish on this PR. Unit tests will catch them all, I just didn't want to invest in writing a bunch of tests before we knew if we'd be merging this or not :)



@dataclass
class _HomogeneousPagingQuery:
query: _PagingQuery
page_from_rows: Callable[[list[Row[_TP]]], Page[Row[_TP]]]


def _prepare_homogeneous_page(
query: PageRequest[_TP], page_identifier: int
) -> _HomogeneousPagingQuery:
"""page_identifier MUST be a trusted int, as it is not escaped."""
# Should have no effect if called correctly, but let's be extra safe.
page_identifier = int(page_identifier)
place, backwards = process_args(query.after, query.before, query.page)

# Grab result_type and keys before adding the _page_identifier so that
# it isn't included in the results.
result_type = orm_result_type(query.query)
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these query.querys are a little confusing, I think we should avoid overloading the word "query". Let's rename _HomogeneousPagingQuery -> _PagingRequest and query -> request or similar?

Copy link
Author

Choose a reason for hiding this comment

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

I also hate this (and it's even worse above, with a q.query.query). I riffed a little on your suggestion, let me know what you think!

keys = orm_query_keys(query.query)
query.query = query.query.add_columns(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we reference query.query multiple times and then modify it, let's split it out into a separate local variable.

Copy link
Author

Choose a reason for hiding this comment

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

Roger

sa.sql.expression.literal_column(str(page_identifier)).label("_page_identifier")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does .add_columns(page_identifier) not work? Given that we're just trying to select an integer value and not a database column, I'm confused about the need for literal_column.

Copy link
Author

Choose a reason for hiding this comment

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

I'll try this, but I believe literal_column is the correct way to insert a literal into a select. It also allows us to give it a label, which is convenient when we do our collation later.

)

# Could we order by page identifier to do the page collation in the DB?

paging_query = prepare_paging(
q=query.query,
per_page=query.per_page,
place=place,
backwards=backwards,
orm=True,
dialect=query.query.session.get_bind().dialect,
)

def page_from_rows(rows):
return orm_page_from_rows(
paging_query, rows, keys, result_type, per_page, backwards, current_place=place
)

return _HomogeneousPagingQuery(query=paging_query, page_from_rows=page_from_rows)