From ff21c99b3c9c70ec5fefb8ac8bf3be87dcf64c0f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 11 Jul 2023 12:01:26 -0700 Subject: [PATCH 01/64] Impelement get_homogeneous_pages. Unit tests forthcoming once we're directionally aligned. --- sqlakeyset/paging.py | 85 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index c1ed1d33..6850f883 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -3,6 +3,7 @@ from __future__ import annotations from functools import partial +from dataclasses import dataclass from typing import ( Any, List, @@ -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? +@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]]]: + """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)) + + +@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) + keys = orm_query_keys(query.query) + query.query = query.query.add_columns( + sa.sql.expression.literal_column(str(page_identifier)).label("_page_identifier") + ) + + # 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) From eff5d52b7a5c36477c7a9cda30a9776371757d7d Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 08:30:38 -0700 Subject: [PATCH 02/64] Check in unit test for multi-page lookup. --- sqlakeyset/paging.py | 47 +++++++++--------- tests/test_paging.py | 111 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 112 insertions(+), 46 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 6850f883..5b38e3e2 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -22,7 +22,7 @@ from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session from sqlalchemy.orm.query import Query -from sqlalchemy.sql.expression import ColumnElement +from sqlalchemy.sql.expression import ColumnElement, literal_column from sqlalchemy.sql.selectable import Select from .columns import OC, MappedOrderColumn, find_order_key, parse_ob_clause @@ -441,7 +441,6 @@ def get_page( return orm_get_page(query, per_page, place, backwards) -# Do we need to support python 3.6? @dataclass class PageRequest(Generic[_TP]): """See ``get_page()`` documentation for parameter explanations.""" @@ -452,7 +451,7 @@ class PageRequest(Generic[_TP]): page: Optional[Union[MarkerLike, str]] = None -def get_homogeneous_page(queries: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]: +def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]: """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 @@ -462,59 +461,61 @@ def get_homogeneous_page(queries: list[PageRequest[_TP]]) -> list[Page[Row[_TP]] Resulting pages are returned in the same order as the original page requests. """ - if not queries: + if not requests: return [] - paging_queries = [_prepare_homogeneous_page(query, i) for i, query in enumerate(queries)] + prepared_queries = [_prepare_homogeneous_page(request, i) for i, query in enumerate(requests)] - query = paging_queries[0].query.query - query = query.union_all(*[q.query.query for q in paging_queries[1:]]) + query = prepared_queries[0].paging_query.query + query = query.union_all(*[p.prepared_query.query for p in prepared_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))} + page_to_rows = {i: list() for i in range(len(requests))} for row in results: page_to_rows[row._page_identifier].append(row) pages = [] - for i in range(len(queries)): + for i in range(len(requests)): rows = page_to_rows[i] - pages.append(paging_queries[i].page_from_rows(rows)) + pages.append(prepared_queries[i].page_from_rows(rows)) + return pages @dataclass -class _HomogeneousPagingQuery: - query: _PagingQuery +class _PreparedQuery: + paging_query: _PagingQuery page_from_rows: Callable[[list[Row[_TP]]], Page[Row[_TP]]] def _prepare_homogeneous_page( - query: PageRequest[_TP], page_identifier: int -) -> _HomogeneousPagingQuery: + request: PageRequest[_TP], page_identifier: int +) -> _PreparedQuery: """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) + place, backwards = process_args(request.after, request.before, request.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) - keys = orm_query_keys(query.query) - query.query = query.query.add_columns( - sa.sql.expression.literal_column(str(page_identifier)).label("_page_identifier") + query = request.query + result_type = orm_result_type(query) + keys = orm_query_keys(query) + query = query.add_columns( + literal_column(str(page_identifier)).label("_page_identifier") ) # 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, + q=query, + per_page=request.per_page, place=place, backwards=backwards, orm=True, - dialect=query.query.session.get_bind().dialect, + dialect=query.session.get_bind().dialect, ) def page_from_rows(rows): @@ -522,4 +523,4 @@ def page_from_rows(rows): paging_query, rows, keys, result_type, per_page, backwards, current_place=place ) - return _HomogeneousPagingQuery(query=paging_query, page_from_rows=page_from_rows) + return _PreparedQuery(paging_query=paging_query, page_from_rows=page_from_rows) diff --git a/tests/test_paging.py b/tests/test_paging.py index 721224f5..f21dd350 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -14,10 +14,11 @@ valid sqlalchemy versions are 1.3.0, 1.4.0, 2.0.0.)""" import warnings from packaging import version +from typing import Any import pytest import sqlalchemy -from sqlalchemy.orm import sessionmaker, aliased, Bundle +from sqlalchemy.orm import sessionmaker, aliased, Bundle, Query from sqlalchemy import ( desc, func, @@ -25,12 +26,16 @@ from sqlakeyset import ( get_page, + get_homogeneous_pages, select_page, serialize_bookmark, unserialize_bookmark, InvalidPage, + PageRequest, ) from sqlakeyset.paging import process_args +from sqlakeyset.results import Page +from sqlakeyset.types import MarkerLike from conftest import ( Book, Author, @@ -49,6 +54,76 @@ warnings.simplefilter("error") +@dataclass +class _PageTracker: + query: Query + unpaged: list + gathered: list + backwards: bool + page: tuple[MarkerLike | str, bool] + page_with_paging: Page | None = None + + +def assert_paging_orm(page_with_paging, gathered, backwards, unpaged): + """Returns the next page, or None if no further pages.""" + paging = page_with_paging.paging + + assert paging.current == page + + if backwards: + gathered = page_with_paging + gathered + else: + gathered = gathered + page_with_paging + + if len(gathered) < len(unpaged): + # Ensure each page is the correct size + assert paging.has_further + assert len(page_with_paging) == per_page + else: + assert not paging.has_further + + if not page_with_paging: + assert not paging.has_further + assert paging.further == paging.current + assert paging.current_opposite == (None, not paging.backwards) + # Is this return None necessary or will paging.further just be None? + return None + + return paging.further + + +def check_multiple_paging_orm(qs): + page_trackers = [ + _PageTracker( + query=q, + gathered=[], + backwards=(i % 2 == 0), + page=(None, i % 2 == 0), + unpaged=q.all(), + ) + for q in qs + ] + while True: + for t in page_tackers: + t.page = unserialize_bookmark(serialize_bookmark(t.page)) + + page_requests = [ + PageRequest(query=t.query, per_page=i, page=t.page) for i, t in enumerate(page_trackers) + ] + pages_with_paging = get_homogeneous_pages(page_requests) + for p, t in zip(pages_with_paging, page_trackers): + page_trackers.page_with_paging = p + + for t in list(page_tackers): + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged) + if page is None: + # Ensure union of pages is original q.all() + assert t.gathered == t.unpaged + page_trackers.remove(t) + + t.page = page + + def check_paging_orm(q): item_counts = range(1, 12) @@ -65,28 +140,8 @@ def check_paging_orm(q): page = unserialize_bookmark(serialized_page) page_with_paging = get_page(q, per_page=per_page, page=serialized_page) - paging = page_with_paging.paging - - assert paging.current == page - - if backwards: - gathered = page_with_paging + gathered - else: - gathered = gathered + page_with_paging - - page = paging.further - - if len(gathered) < len(unpaged): - # Ensure each page is the correct size - assert paging.has_further - assert len(page_with_paging) == per_page - else: - assert not paging.has_further - - if not page_with_paging: - assert not paging.has_further - assert paging.further == paging.current - assert paging.current_opposite == (None, not paging.backwards) + page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged) + if page is None: break # Ensure union of pages is original q.all() @@ -361,6 +416,16 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): check_paging_orm(q=q) +def test_orm_multiple_pages(dburl): + with S(dburl, echo=ECHO) as s: + qs = [ + s.query(Animal).order_by(Animal.leg_count, Animal.id), + s.query(Animal).filter(Animal.leg_count == 4).order_by(Animal.id), + s.query(Animal).order_by(Animal.leg_count, Animal.id.desc()), + ] + check_multiple_paging_orm(qs=qs) + + def test_core(dburl): selectable = ( select(Book.b, Book.d, Book.id, Book.c) From d37132bdc8ddc2e51d4d47cc44b9c52eed3e5da2 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 08:34:44 -0700 Subject: [PATCH 03/64] Fix missing symbols. --- sqlakeyset/paging.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 5b38e3e2..14f71ba2 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -6,6 +6,8 @@ from dataclasses import dataclass from typing import ( Any, + Callable, + Generic, List, NamedTuple, Optional, @@ -449,7 +451,7 @@ class PageRequest(Generic[_TP]): after: OptionalKeyset = None before: OptionalKeyset = None page: Optional[Union[MarkerLike, str]] = None - + def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]: """Get multiple pages of results for homogeneous legacy ORM queries. @@ -464,7 +466,7 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP if not requests: return [] - prepared_queries = [_prepare_homogeneous_page(request, i) for i, query in enumerate(requests)] + prepared_queries = [_prepare_homogeneous_page(request, i) for i, request in enumerate(requests)] query = prepared_queries[0].paging_query.query query = query.union_all(*[p.prepared_query.query for p in prepared_queries[1:]]) @@ -520,7 +522,7 @@ def _prepare_homogeneous_page( def page_from_rows(rows): return orm_page_from_rows( - paging_query, rows, keys, result_type, per_page, backwards, current_place=place + paging_query, rows, keys, result_type, request.per_page, backwards, current_place=place ) return _PreparedQuery(paging_query=paging_query, page_from_rows=page_from_rows) From df7466cdb3019526af8d7f48990118a1183d7353 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 09:39:22 -0700 Subject: [PATCH 04/64] Fix flake8 test_paging.py --- tests/test_paging.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index f21dd350..b6295586 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -13,8 +13,8 @@ docker containers. (Available python versions are 3.7, 3.8, 3.9, 3.10, 3.11 and valid sqlalchemy versions are 1.3.0, 1.4.0, 2.0.0.)""" import warnings +from dataclasses import dataclass from packaging import version -from typing import Any import pytest import sqlalchemy @@ -64,11 +64,11 @@ class _PageTracker: page_with_paging: Page | None = None -def assert_paging_orm(page_with_paging, gathered, backwards, unpaged): +def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, per_page): """Returns the next page, or None if no further pages.""" paging = page_with_paging.paging - assert paging.current == page + assert paging.current == page_with_paging.page if backwards: gathered = page_with_paging + gathered @@ -101,10 +101,10 @@ def check_multiple_paging_orm(qs): page=(None, i % 2 == 0), unpaged=q.all(), ) - for q in qs + for i, q in enumerate(qs) ] while True: - for t in page_tackers: + for t in page_trackers: t.page = unserialize_bookmark(serialize_bookmark(t.page)) page_requests = [ @@ -114,8 +114,8 @@ def check_multiple_paging_orm(qs): for p, t in zip(pages_with_paging, page_trackers): page_trackers.page_with_paging = p - for t in list(page_tackers): - page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged) + for i, t in enumerate(list(page_trackers)): + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, i) if page is None: # Ensure union of pages is original q.all() assert t.gathered == t.unpaged @@ -140,7 +140,7 @@ def check_paging_orm(q): page = unserialize_bookmark(serialized_page) page_with_paging = get_page(q, per_page=per_page, page=serialized_page) - page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged) + page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged, per_page) if page is None: break From 4843e68e8e623f594758d6c279ee344c84f05db8 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:05:35 -0700 Subject: [PATCH 05/64] Export get_homogeneous_pages. --- sqlakeyset/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sqlakeyset/__init__.py b/sqlakeyset/__init__.py index 322327c0..7d054cf2 100644 --- a/sqlakeyset/__init__.py +++ b/sqlakeyset/__init__.py @@ -16,6 +16,7 @@ __all__ = [ "get_page", + "get_homogeneous_pages", "select_page", "serialize_bookmark", "unserialize_bookmark", From 9985e4ed6423cdf18573c095cd4c91665b90d277 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:11:52 -0700 Subject: [PATCH 06/64] Actually import get_homogeneous_pages. --- sqlakeyset/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sqlakeyset/__init__.py b/sqlakeyset/__init__.py index 7d054cf2..63f55a6d 100644 --- a/sqlakeyset/__init__.py +++ b/sqlakeyset/__init__.py @@ -1,4 +1,4 @@ -from .paging import get_page, select_page, InvalidPage +from .paging import get_homogeneous_pages, get_page, select_page, InvalidPage from .results import ( Page, Paging, @@ -15,8 +15,8 @@ from .types import Keyset, Marker __all__ = [ - "get_page", "get_homogeneous_pages", + "get_page", "select_page", "serialize_bookmark", "unserialize_bookmark", From bd486e10a823f06b5a428b3f774cf5d9e29e8d81 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:14:05 -0700 Subject: [PATCH 07/64] Add PageRequest. --- sqlakeyset/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/__init__.py b/sqlakeyset/__init__.py index 63f55a6d..6b286d8d 100644 --- a/sqlakeyset/__init__.py +++ b/sqlakeyset/__init__.py @@ -1,4 +1,4 @@ -from .paging import get_homogeneous_pages, get_page, select_page, InvalidPage +from .paging import get_homogeneous_pages, get_page, select_page, InvalidPage, PageRequest from .results import ( Page, Paging, @@ -21,6 +21,7 @@ "serialize_bookmark", "unserialize_bookmark", "Page", + "PageRequest", "Paging", "Keyset", "Marker", From e53424376c6cecf00fb333c51f78b39ab0975583 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:17:32 -0700 Subject: [PATCH 08/64] Fix bad symbol. --- tests/test_paging.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index b6295586..6230042c 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -64,11 +64,11 @@ class _PageTracker: page_with_paging: Page | None = None -def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, per_page): +def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page): """Returns the next page, or None if no further pages.""" paging = page_with_paging.paging - assert paging.current == page_with_paging.page + assert paging.current == page if backwards: gathered = page_with_paging + gathered @@ -115,7 +115,7 @@ def check_multiple_paging_orm(qs): page_trackers.page_with_paging = p for i, t in enumerate(list(page_trackers)): - page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, i) + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i) if page is None: # Ensure union of pages is original q.all() assert t.gathered == t.unpaged @@ -140,7 +140,7 @@ def check_paging_orm(q): page = unserialize_bookmark(serialized_page) page_with_paging = get_page(q, per_page=per_page, page=serialized_page) - page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged, per_page) + page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page) if page is None: break From 5e6fd6a34d55c5144a89fd812e690cff49e8bec6 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:22:09 -0700 Subject: [PATCH 09/64] Extend gathered. --- tests/test_paging.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 6230042c..9ecf0bf0 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -65,15 +65,18 @@ class _PageTracker: def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page): - """Returns the next page, or None if no further pages.""" + """Returns the next page, or None if no further pages. + + Modifies gathered in place. + """ paging = page_with_paging.paging assert paging.current == page if backwards: - gathered = page_with_paging + gathered + gathered[:0] = page_with_paging + gathered else: - gathered = gathered + page_with_paging + gathered.extend(page_with_paging) if len(gathered) < len(unpaged): # Ensure each page is the correct size From f5075c3418bb3c6f26d70d0304e55799224ea21a Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:24:55 -0700 Subject: [PATCH 10/64] Use Union instead of |. --- tests/test_paging.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 9ecf0bf0..f3495010 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -15,6 +15,7 @@ import warnings from dataclasses import dataclass from packaging import version +from typing import Union import pytest import sqlalchemy @@ -60,7 +61,7 @@ class _PageTracker: unpaged: list gathered: list backwards: bool - page: tuple[MarkerLike | str, bool] + page: tuple[Union[MarkerLike, str], bool] page_with_paging: Page | None = None @@ -74,7 +75,7 @@ def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_ assert paging.current == page if backwards: - gathered[:0] = page_with_paging + gathered + gathered[:0] = page_with_paging else: gathered.extend(page_with_paging) From 9aae97fcc14503a8832e82fa0e07a22da98cf4ba Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:26:41 -0700 Subject: [PATCH 11/64] Use Tuple instead of tuple. --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index f3495010..9aebd226 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -15,7 +15,7 @@ import warnings from dataclasses import dataclass from packaging import version -from typing import Union +from typing import Tuple, Union import pytest import sqlalchemy @@ -61,7 +61,7 @@ class _PageTracker: unpaged: list gathered: list backwards: bool - page: tuple[Union[MarkerLike, str], bool] + page: Tuple[Union[MarkerLike, str], bool] page_with_paging: Page | None = None From cbefa301e09ada21dd97516f7ed50b1c9b4b6c76 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:31:45 -0700 Subject: [PATCH 12/64] Use Book instead of Animal. --- tests/test_paging.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 9aebd226..4fad4629 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -423,9 +423,9 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): def test_orm_multiple_pages(dburl): with S(dburl, echo=ECHO) as s: qs = [ - s.query(Animal).order_by(Animal.leg_count, Animal.id), - s.query(Animal).filter(Animal.leg_count == 4).order_by(Animal.id), - s.query(Animal).order_by(Animal.leg_count, Animal.id.desc()), + s.query(Book).order_by(Book.author_id, Book.id), + s.query(Book).filter(Book.author_id < 4).order_by(Book.id), + s.query(Book).order_by(Book.author_id, Book.id.desc()), ] check_multiple_paging_orm(qs=qs) From bb86b7e5c85567890ca7f1e734a8b40263e84a7b Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:34:20 -0700 Subject: [PATCH 13/64] More python 3.7 and fix wrong symbol name. --- sqlakeyset/paging.py | 2 +- tests/test_paging.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 14f71ba2..952f407b 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -469,7 +469,7 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP prepared_queries = [_prepare_homogeneous_page(request, i) for i, request in enumerate(requests)] query = prepared_queries[0].paging_query.query - query = query.union_all(*[p.prepared_query.query for p in prepared_queries[1:]]) + query = query.union_all(*[p.paging_query.query for p in prepared_queries[1:]]) results = query.all() diff --git a/tests/test_paging.py b/tests/test_paging.py index 4fad4629..6399f46c 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -15,7 +15,7 @@ import warnings from dataclasses import dataclass from packaging import version -from typing import Tuple, Union +from typing import Optional, Tuple, Union import pytest import sqlalchemy @@ -62,7 +62,7 @@ class _PageTracker: gathered: list backwards: bool page: Tuple[Union[MarkerLike, str], bool] - page_with_paging: Page | None = None + page_with_paging: Optional[Page] = None def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page): From cdd0fc23c4231b904492432d8eec82d6a651232b Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:38:20 -0700 Subject: [PATCH 14/64] Improve tests. --- tests/test_paging.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 6399f46c..350fff95 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -424,10 +424,23 @@ def test_orm_multiple_pages(dburl): with S(dburl, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.author_id, Book.id), - s.query(Book).filter(Book.author_id < 4).order_by(Book.id), + s.query(Book).filter(Book.author_id == 1).order_by(Book.id), s.query(Book).order_by(Book.author_id, Book.id.desc()), ] - check_multiple_paging_orm(qs=qs) + check_multiple_paging_orm(qs=qs) + + +def test_orm_multiple_pages_one_query(dburl): + with S(dburl, echo=ECHO) as s: + qs = [ + s.query(Book).order_by(Book.id), + ] + check_multiple_paging_orm(qs=qs) + + +def test_orm_multiple_pages_empty_queries(dburl): + with S(dburl, echo=ECHO) as s: + assert get_homogeneous_pages([]) == [] def test_core(dburl): From 2af1ca2a6181e446c5bfe5e4c37b12d250476eb1 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:40:45 -0700 Subject: [PATCH 15/64] Remove unused db session. --- tests/test_paging.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 350fff95..f68991ff 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -439,8 +439,7 @@ def test_orm_multiple_pages_one_query(dburl): def test_orm_multiple_pages_empty_queries(dburl): - with S(dburl, echo=ECHO) as s: - assert get_homogeneous_pages([]) == [] + assert get_homogeneous_pages([]) == [] def test_core(dburl): From 76c2939d9312ec4498741c52ccd5ac0a3bd2823e Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:52:36 -0700 Subject: [PATCH 16/64] Exclude sqlite. --- sqlakeyset/paging.py | 3 +++ tests/conftest.py | 1 + tests/test_paging.py | 6 +++--- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 952f407b..eb158cb7 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -461,6 +461,9 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP same columns. They may have different filters or ordering, but must result in selecting the same columns with the same names. + Note: This requires the underlying database to support ORDER BY and LIMIT + statements in components of a compound select, which SQLite does not. + Resulting pages are returned in the same order as the original page requests. """ if not requests: diff --git a/tests/conftest.py b/tests/conftest.py index 37421080..4d264386 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -246,6 +246,7 @@ def _dburl(request): dburl = pytest.fixture(params=SUPPORTED_ENGINES)(_dburl) no_mysql_dburl = pytest.fixture(params=["sqlite", "postgresql"])(_dburl) +no_sqlite_dburl = pytest.fixture(params=["mysql", "postgresql"])(_dburl) pg_only_dburl = pytest.fixture(params=["postgresql"])(_dburl) diff --git a/tests/test_paging.py b/tests/test_paging.py index f68991ff..8da4e5e6 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -420,7 +420,7 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): check_paging_orm(q=q) -def test_orm_multiple_pages(dburl): +def test_orm_multiple_pages(no_sqlite_url): with S(dburl, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.author_id, Book.id), @@ -430,7 +430,7 @@ def test_orm_multiple_pages(dburl): check_multiple_paging_orm(qs=qs) -def test_orm_multiple_pages_one_query(dburl): +def test_orm_multiple_pages_one_query(no_sqlite_url): with S(dburl, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.id), @@ -438,7 +438,7 @@ def test_orm_multiple_pages_one_query(dburl): check_multiple_paging_orm(qs=qs) -def test_orm_multiple_pages_empty_queries(dburl): +def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] From 6867d838d8b9311b9a47a63873377e6552137c9e Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:54:52 -0700 Subject: [PATCH 17/64] s/dburl/no_sqlite_url --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 8da4e5e6..0764cbcd 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -421,7 +421,7 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): def test_orm_multiple_pages(no_sqlite_url): - with S(dburl, echo=ECHO) as s: + with S(no_sqlite_url, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.author_id, Book.id), s.query(Book).filter(Book.author_id == 1).order_by(Book.id), @@ -431,7 +431,7 @@ def test_orm_multiple_pages(no_sqlite_url): def test_orm_multiple_pages_one_query(no_sqlite_url): - with S(dburl, echo=ECHO) as s: + with S(no_sqlite_url, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.id), ] From 19ff0099048f5cf066d3c87e47bbd99efd54f4b7 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 15:57:43 -0700 Subject: [PATCH 18/64] s/url/dburl --- tests/test_paging.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 0764cbcd..5432c95a 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -420,8 +420,8 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): check_paging_orm(q=q) -def test_orm_multiple_pages(no_sqlite_url): - with S(no_sqlite_url, echo=ECHO) as s: +def test_orm_multiple_pages(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.author_id, Book.id), s.query(Book).filter(Book.author_id == 1).order_by(Book.id), @@ -430,8 +430,8 @@ def test_orm_multiple_pages(no_sqlite_url): check_multiple_paging_orm(qs=qs) -def test_orm_multiple_pages_one_query(no_sqlite_url): - with S(no_sqlite_url, echo=ECHO) as s: +def test_orm_multiple_pages_one_query(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ s.query(Book).order_by(Book.id), ] From abdef8af139c050d8d7212b63a800878edcb6029 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Thu, 13 Jul 2023 16:01:12 -0700 Subject: [PATCH 19/64] Fix for loop variable. --- tests/test_paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 5432c95a..6bb80f8f 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -116,7 +116,7 @@ def check_multiple_paging_orm(qs): ] pages_with_paging = get_homogeneous_pages(page_requests) for p, t in zip(pages_with_paging, page_trackers): - page_trackers.page_with_paging = p + t.page_with_paging = p for i, t in enumerate(list(page_trackers)): page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i) From a5514cfd8195fcd6eb2ef03c44e1256fcdf4f13e Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 06:32:33 -0700 Subject: [PATCH 20/64] Per page should be at least 1. --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 6bb80f8f..ef582e4e 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -112,14 +112,14 @@ def check_multiple_paging_orm(qs): t.page = unserialize_bookmark(serialize_bookmark(t.page)) page_requests = [ - PageRequest(query=t.query, per_page=i, page=t.page) for i, t in enumerate(page_trackers) + PageRequest(query=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) ] pages_with_paging = get_homogeneous_pages(page_requests) for p, t in zip(pages_with_paging, page_trackers): t.page_with_paging = p for i, t in enumerate(list(page_trackers)): - page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i) + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 1) if page is None: # Ensure union of pages is original q.all() assert t.gathered == t.unpaged From b7a34f5d764cc7ada3d2c78393baf012dc76c4be Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 06:57:09 -0700 Subject: [PATCH 21/64] Fix UNION ALL ordering which isn't guaranteed. --- sqlakeyset/paging.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index eb158cb7..6a4481c0 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -19,7 +19,7 @@ ) from typing_extensions import Literal # to keep python 3.7 support -from sqlalchemy import tuple_, and_, or_ +from sqlalchemy import tuple_, and_, or_, func, text from sqlalchemy.engine import Connection from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session @@ -472,8 +472,9 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP prepared_queries = [_prepare_homogeneous_page(request, i) for i, request in enumerate(requests)] query = prepared_queries[0].paging_query.query - query = query.union_all(*[p.paging_query.query for p in prepared_queries[1:]]) + query = query.union_all(*[p.paging_query.query for p in prepared_queries[1:]]).order_by(text("_page_identifier"), text("_row_number")) + print(query) results = query.all() # We need to make sure there's an entry for every page in case some return @@ -508,9 +509,6 @@ def _prepare_homogeneous_page( query = request.query result_type = orm_result_type(query) keys = orm_query_keys(query) - query = query.add_columns( - literal_column(str(page_identifier)).label("_page_identifier") - ) # Could we order by page identifier to do the page collation in the DB? @@ -522,6 +520,13 @@ def _prepare_homogeneous_page( orm=True, dialect=query.session.get_bind().dialect, ) + query = paging_query.query + paging_query.query = paging_query.query.add_columns( + literal_column(str(page_identifier)).label("_page_identifier"), + func.ROW_NUMBER().over( + order_by=[c.element for c in paging_query.order_columns] + ).label("_row_number") + ) def page_from_rows(rows): return orm_page_from_rows( From 2f4f4bd92bacac07a1dc82c08728d428dbf3fa3f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:02:28 -0700 Subject: [PATCH 22/64] NamedTuple is immutable. --- sqlakeyset/paging.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 6a4481c0..f08d7570 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -510,6 +510,14 @@ def _prepare_homogeneous_page( result_type = orm_result_type(query) keys = orm_query_keys(query) + # This is unfortunately duplicated with prepare_paging, but we need + """ + selectable = orm_to_selectable(query) + order_cols = parse_ob_clause(selectable) + if backwards: + order_cols = [c.reversed for c in order_cols] + """ + # Could we order by page identifier to do the page collation in the DB? paging_query = prepare_paging( @@ -521,12 +529,13 @@ def _prepare_homogeneous_page( dialect=query.session.get_bind().dialect, ) query = paging_query.query - paging_query.query = paging_query.query.add_columns( + query = query.add_columns( literal_column(str(page_identifier)).label("_page_identifier"), func.ROW_NUMBER().over( order_by=[c.element for c in paging_query.order_columns] ).label("_row_number") ) + paging_query = _PagingQuery(query, *paging_query[1:]) def page_from_rows(rows): return orm_page_from_rows( From 3909554c4671e580818342a18c8a22688edb4bca Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:05:26 -0700 Subject: [PATCH 23/64] Trailing whitespace. --- sqlakeyset/paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index f08d7570..38a7df24 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -510,7 +510,7 @@ def _prepare_homogeneous_page( result_type = orm_result_type(query) keys = orm_query_keys(query) - # This is unfortunately duplicated with prepare_paging, but we need + # This is unfortunately duplicated with prepare_paging, but we need """ selectable = orm_to_selectable(query) order_cols = parse_ob_clause(selectable) From 3393641d12aca68542d8202cd2c1bb92c1e0a54f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:16:03 -0700 Subject: [PATCH 24/64] Use uo instead of element to get ordering right for ROW_NUMBER. --- sqlakeyset/paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 38a7df24..f3f08c35 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -532,7 +532,7 @@ def _prepare_homogeneous_page( query = query.add_columns( literal_column(str(page_identifier)).label("_page_identifier"), func.ROW_NUMBER().over( - order_by=[c.element for c in paging_query.order_columns] + order_by=[c.uo for c in paging_query.order_columns] ).label("_row_number") ) paging_query = _PagingQuery(query, *paging_query[1:]) From f985063a662878547322f5375c4e5578be3da41c Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:24:48 -0700 Subject: [PATCH 25/64] Use deque for extendleft functionality. --- tests/test_paging.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index ef582e4e..27702734 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -13,6 +13,7 @@ docker containers. (Available python versions are 3.7, 3.8, 3.9, 3.10, 3.11 and valid sqlalchemy versions are 1.3.0, 1.4.0, 2.0.0.)""" import warnings +from collections import deque from dataclasses import dataclass from packaging import version from typing import Optional, Tuple, Union @@ -59,7 +60,7 @@ class _PageTracker: query: Query unpaged: list - gathered: list + gathered: deque backwards: bool page: Tuple[Union[MarkerLike, str], bool] page_with_paging: Optional[Page] = None @@ -75,7 +76,7 @@ def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_ assert paging.current == page if backwards: - gathered[:0] = page_with_paging + gathered.extendleft(reversed(page_with_paging)) else: gathered.extend(page_with_paging) @@ -100,7 +101,7 @@ def check_multiple_paging_orm(qs): page_trackers = [ _PageTracker( query=q, - gathered=[], + gathered=deque(), backwards=(i % 2 == 0), page=(None, i % 2 == 0), unpaged=q.all(), @@ -122,7 +123,7 @@ def check_multiple_paging_orm(qs): page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 1) if page is None: # Ensure union of pages is original q.all() - assert t.gathered == t.unpaged + assert list(t.gathered) == t.unpaged page_trackers.remove(t) t.page = page @@ -135,7 +136,7 @@ def check_paging_orm(q): for backwards in [False, True]: for per_page in item_counts: - gathered = [] + gathered = deque() page = None, backwards @@ -149,7 +150,7 @@ def check_paging_orm(q): break # Ensure union of pages is original q.all() - assert gathered == unpaged + assert list(gathered) == unpaged def check_paging_core(selectable, s): From e5feac2c5f9e014b4adcb58b1e607e34513de0ff Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:30:07 -0700 Subject: [PATCH 26/64] Debugging info. --- sqlakeyset/paging.py | 1 - tests/test_paging.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index f3f08c35..0f211f42 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -474,7 +474,6 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP query = prepared_queries[0].paging_query.query query = query.union_all(*[p.paging_query.query for p in prepared_queries[1:]]).order_by(text("_page_identifier"), text("_row_number")) - print(query) results = query.all() # We need to make sure there's an entry for every page in case some return diff --git a/tests/test_paging.py b/tests/test_paging.py index 27702734..a0e440f3 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -81,9 +81,10 @@ def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_ gathered.extend(page_with_paging) if len(gathered) < len(unpaged): + print(f"{len(gathered)} < {len(unpaged)}") # Ensure each page is the correct size - assert paging.has_further assert len(page_with_paging) == per_page + assert paging.has_further else: assert not paging.has_further From 46248d252f7a1da5d0c9438dfad11aab2ffede20 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:43:04 -0700 Subject: [PATCH 27/64] Testing. --- tests/test_paging.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index a0e440f3..c76e7080 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -114,14 +114,15 @@ def check_multiple_paging_orm(qs): t.page = unserialize_bookmark(serialize_bookmark(t.page)) page_requests = [ - PageRequest(query=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) + PageRequest(query=t.query, per_page=i + 2, page=t.page) for i, t in enumerate(page_trackers) ] pages_with_paging = get_homogeneous_pages(page_requests) for p, t in zip(pages_with_paging, page_trackers): t.page_with_paging = p + print(p) for i, t in enumerate(list(page_trackers)): - page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 1) + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 2) if page is None: # Ensure union of pages is original q.all() assert list(t.gathered) == t.unpaged From c5a606e065a7b1cb6952a9cbd81e8a410f904692 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:51:12 -0700 Subject: [PATCH 28/64] Add before preparing paging. --- sqlakeyset/paging.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 0f211f42..17ccf83f 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -510,13 +510,17 @@ def _prepare_homogeneous_page( keys = orm_query_keys(query) # This is unfortunately duplicated with prepare_paging, but we need - """ selectable = orm_to_selectable(query) order_cols = parse_ob_clause(selectable) if backwards: order_cols = [c.reversed for c in order_cols] - """ + query = query.add_columns( + literal_column(str(page_identifier)).label("_page_identifier"), + func.ROW_NUMBER().over( + order_by=[c.uo for c in order_cols] + ).label("_row_number") + ) # Could we order by page identifier to do the page collation in the DB? paging_query = prepare_paging( @@ -528,12 +532,6 @@ def _prepare_homogeneous_page( dialect=query.session.get_bind().dialect, ) query = paging_query.query - query = query.add_columns( - literal_column(str(page_identifier)).label("_page_identifier"), - func.ROW_NUMBER().over( - order_by=[c.uo for c in paging_query.order_columns] - ).label("_row_number") - ) paging_query = _PagingQuery(query, *paging_query[1:]) def page_from_rows(rows): From 3ccdd3e1563984faed0a68e0d984843482496d24 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 07:54:33 -0700 Subject: [PATCH 29/64] author_id is nullable which doesn't work with paging. --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index c76e7080..fea4bd87 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -426,9 +426,9 @@ def test_orm_joined_inheritance(joined_inheritance_dburl): def test_orm_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ - s.query(Book).order_by(Book.author_id, Book.id), + s.query(Book).order_by(Book.name, Book.id), s.query(Book).filter(Book.author_id == 1).order_by(Book.id), - s.query(Book).order_by(Book.author_id, Book.id.desc()), + s.query(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_orm(qs=qs) From 0e7cc0bcfce403720017e93aef7d3f7e8271f71d Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 08:10:28 -0700 Subject: [PATCH 30/64] Actually end the test on success. --- tests/test_paging.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_paging.py b/tests/test_paging.py index fea4bd87..fbaca2d6 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -127,9 +127,13 @@ def check_multiple_paging_orm(qs): # Ensure union of pages is original q.all() assert list(t.gathered) == t.unpaged page_trackers.remove(t) + continue t.page = page + if not page_trackers: + break + def check_paging_orm(q): item_counts = range(1, 12) From 13789365d2a591c01b6c414f138d10f003726c1c Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 09:13:29 -0700 Subject: [PATCH 31/64] Add a test for fetching columns. --- sqlakeyset/paging.py | 6 ++++-- tests/test_paging.py | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 17ccf83f..3cbbac5c 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -504,12 +504,15 @@ def _prepare_homogeneous_page( place, backwards = process_args(request.after, request.before, request.page) # Grab result_type and keys before adding the _page_identifier so that - # it isn't included in the results. + # it isn't included in the results. page_from_rows will slice the resulting + # row based on the length of keys query = request.query result_type = orm_result_type(query) keys = orm_query_keys(query) # This is unfortunately duplicated with prepare_paging, but we need + # to get it to add the columns *before* we do the other query wrangling + # inside prepare_paging. selectable = orm_to_selectable(query) order_cols = parse_ob_clause(selectable) if backwards: @@ -521,7 +524,6 @@ def _prepare_homogeneous_page( order_by=[c.uo for c in order_cols] ).label("_row_number") ) - # Could we order by page identifier to do the page collation in the DB? paging_query = prepare_paging( q=query, diff --git a/tests/test_paging.py b/tests/test_paging.py index fbaca2d6..1021adf6 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -81,7 +81,6 @@ def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_ gathered.extend(page_with_paging) if len(gathered) < len(unpaged): - print(f"{len(gathered)} < {len(unpaged)}") # Ensure each page is the correct size assert len(page_with_paging) == per_page assert paging.has_further @@ -114,15 +113,14 @@ def check_multiple_paging_orm(qs): t.page = unserialize_bookmark(serialize_bookmark(t.page)) page_requests = [ - PageRequest(query=t.query, per_page=i + 2, page=t.page) for i, t in enumerate(page_trackers) + PageRequest(query=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) ] pages_with_paging = get_homogeneous_pages(page_requests) for p, t in zip(pages_with_paging, page_trackers): t.page_with_paging = p - print(p) for i, t in enumerate(list(page_trackers)): - page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 2) + page = assert_paging_orm(t.page_with_paging, t.gathered, t.backwards, t.unpaged, t.page, i + 1) if page is None: # Ensure union of pages is original q.all() assert list(t.gathered) == t.unpaged @@ -437,6 +435,16 @@ def test_orm_multiple_pages(no_sqlite_dburl): check_multiple_paging_orm(qs=qs) +def test_orm_multiple_pages_select_columns(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: + qs = [ + s.query(Book).order_by(Book.name, Book.id), + s.query(Book.name, Book.id).filter(Book.author_id == 1).order_by(Book.id), + s.query(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), + ] + check_multiple_paging_orm(qs=qs) + + def test_orm_multiple_pages_one_query(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ From b610c69435fafb7604fbb3d71834a47064409fbe Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 09:18:06 -0700 Subject: [PATCH 32/64] Make test homogeneous. --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 1021adf6..5124d267 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -438,8 +438,8 @@ def test_orm_multiple_pages(no_sqlite_dburl): def test_orm_multiple_pages_select_columns(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ - s.query(Book).order_by(Book.name, Book.id), - s.query(Book.name, Book.id).filter(Book.author_id == 1).order_by(Book.id), + s.query(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id), + s.query(Book.name, Book.author_id, Book.id).filter(Book.author_id == 1).order_by(Book.id), s.query(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_orm(qs=qs) From d46719f8e754479d3885f7516dac938f7190c6eb Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 09:30:08 -0700 Subject: [PATCH 33/64] Move page_identifier inside prepare_paging. --- sqlakeyset/paging.py | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 3cbbac5c..4bebeecc 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -24,7 +24,7 @@ from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session from sqlalchemy.orm.query import Query -from sqlalchemy.sql.expression import ColumnElement, literal_column +from sqlalchemy.sql.expression import ColumnElement, literal from sqlalchemy.sql.selectable import Select from .columns import OC, MappedOrderColumn, find_order_key, parse_ob_clause @@ -155,6 +155,7 @@ def prepare_paging( backwards: bool, orm: Literal[True], dialect: Dialect, + page_identifier: Optional[int] = None, ) -> _PagingQuery: ... @@ -167,6 +168,7 @@ def prepare_paging( backwards: bool, orm: Literal[False], dialect: Dialect, + page_identifier: Optional[int] = None, ) -> _PagingSelect: ... @@ -178,6 +180,7 @@ def prepare_paging( backwards: bool, orm: bool, dialect: Dialect, + page_identifier: Optional[int] = None, ) -> Union[_PagingQuery, _PagingSelect]: if orm: if not isinstance(q, Query): @@ -206,6 +209,15 @@ def prepare_paging( extra_columns = [ col.extra_column for col in mapped_ocols if col.extra_column is not None ] + + # page_identifier is used for fetching multiple pages. + if page_identifier is not None: + extra_columns += [ + literal(page_identifier).label("_page_identifier"), + func.ROW_NUMBER().over( + order_by=[c.uo for c in order_cols] + ).label("_row_number"), + ] if hasattr(q, "add_columns"): # ORM or SQLAlchemy 1.4+ q = q.add_columns(*extra_columns) else: @@ -498,33 +510,12 @@ class _PreparedQuery: def _prepare_homogeneous_page( request: PageRequest[_TP], page_identifier: int ) -> _PreparedQuery: - """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(request.after, request.before, request.page) - # Grab result_type and keys before adding the _page_identifier so that - # it isn't included in the results. page_from_rows will slice the resulting - # row based on the length of keys query = request.query result_type = orm_result_type(query) keys = orm_query_keys(query) - # This is unfortunately duplicated with prepare_paging, but we need - # to get it to add the columns *before* we do the other query wrangling - # inside prepare_paging. - selectable = orm_to_selectable(query) - order_cols = parse_ob_clause(selectable) - if backwards: - order_cols = [c.reversed for c in order_cols] - - query = query.add_columns( - literal_column(str(page_identifier)).label("_page_identifier"), - func.ROW_NUMBER().over( - order_by=[c.uo for c in order_cols] - ).label("_row_number") - ) - paging_query = prepare_paging( q=query, per_page=request.per_page, @@ -532,9 +523,8 @@ def _prepare_homogeneous_page( backwards=backwards, orm=True, dialect=query.session.get_bind().dialect, + page_identifier=page_identifier, ) - query = paging_query.query - paging_query = _PagingQuery(query, *paging_query[1:]) def page_from_rows(rows): return orm_page_from_rows( From bdac930c82ea4c4c09843b6160eb7ff448c0609d Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:09:11 -0700 Subject: [PATCH 34/64] Add select_homogeneous_pages and refactor tests. --- sqlakeyset/__init__.py | 2 +- sqlakeyset/paging.py | 107 +++++++++++++++++++++++++++++++++++++---- tests/test_paging.py | 43 ++++++++++------- 3 files changed, 123 insertions(+), 29 deletions(-) diff --git a/sqlakeyset/__init__.py b/sqlakeyset/__init__.py index 6b286d8d..4996f35e 100644 --- a/sqlakeyset/__init__.py +++ b/sqlakeyset/__init__.py @@ -1,4 +1,4 @@ -from .paging import get_homogeneous_pages, get_page, select_page, InvalidPage, PageRequest +from .paging import get_homogeneous_pages, get_page, select_homogeneous_pages, select_page, InvalidPage, OrmPageRequest, PageRequest from .results import ( Page, Paging, diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 4bebeecc..48da5c32 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -291,11 +291,13 @@ def core_get_page( :param backwards: If ``True``, reverse pagination direction. :returns: :class:`Page` """ - # We need the result schema for the *original* query in order to properly - # trim off our extra_columns. As far as I can tell, this is the only + # In SQLAlchemy 1.3, we need the result schema for the *original* query in order + # to properly trim off our extra_columns. As far as I can tell, this is the only # way to get it without copy-pasting chunks of the sqlalchemy internals. # LIMIT 0 to minimize database load (though the fact that a round trip to # the DB has to happen at all is regrettable). + # + # Thankfully this is obsolete in 1.4+ result_type = core_result_type(selectable, s) sel = prepare_paging( q=selectable, @@ -456,7 +458,7 @@ def get_page( @dataclass -class PageRequest(Generic[_TP]): +class OrmPageRequest(Generic[_TP]): """See ``get_page()`` documentation for parameter explanations.""" query: Query[_TP] per_page: int = PER_PAGE_DEFAULT @@ -465,7 +467,17 @@ class PageRequest(Generic[_TP]): page: Optional[Union[MarkerLike, str]] = None -def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]: +@dataclass +class PageRequest(Generic[_TP]): + """See ``select_page()`` documentation for parameter explanations.""" + selectable: Select[_TP] + per_page: int = PER_PAGE_DEFAULT + after: OptionalKeyset = None + before: OptionalKeyset = None + page: Optional[Union[MarkerLike, str]] = None + + +def get_homogeneous_pages(requests: list[OrmPageRequest[_TP]]) -> list[Page[Row[_TP]]]: """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 @@ -481,10 +493,14 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP if not requests: return [] - prepared_queries = [_prepare_homogeneous_page(request, i) for i, request in enumerate(requests)] + prepared_queries = [ + _orm_prepare_homogeneous_page(request, i) for i, request in enumerate(requests) + ] query = prepared_queries[0].paging_query.query - query = query.union_all(*[p.paging_query.query for p in prepared_queries[1:]]).order_by(text("_page_identifier"), text("_row_number")) + query = query.union_all( + *[p.paging_query.query for p in prepared_queries[1:]] + ).order_by(text("_page_identifier"), text("_row_number")) results = query.all() @@ -501,14 +517,85 @@ def get_homogeneous_pages(requests: list[PageRequest[_TP]]) -> list[Page[Row[_TP return pages +def select_homogeneous_pages( + requests: list[PageRequest[_TP]], s: Union[Session, Connection] +) -> list[Page[Row[_TP]]]: + """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. + + Note: This requires the underlying database to support ORDER BY and LIMIT + statements in components of a compound select, which SQLite does not. + + Resulting pages are returned in the same order as the original page requests. + """ + if not requests: + return [] + + prepared_queries = [_core_prepare_homogeneous_page(request, s, i) for i, request in enumerate(requests)] + + select = union_all(*[p.paging_query.select for p in prepared_queries]) + selected = s.execute(sel.select) + + results = query.fetchall() + + # 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(requests))} + for row in results: + page_to_rows[row._page_identifier].append(row) + + pages = [] + for i in range(len(requests)): + rows = page_to_rows[i] + pages.append(prepared_queries[i].page_from_rows(rows, selected)) + return pages + + @dataclass class _PreparedQuery: - paging_query: _PagingQuery - page_from_rows: Callable[[list[Row[_TP]]], Page[Row[_TP]]] + paging_query: Union[_PagingQuery, _PagingSelect] + page_from_rows: Callable[..., Page[Row[_TP]]] + + +def _core_prepare_homogeneous_page( + request: PageRequest[_TP], s: Union[Session, Connection], page_identifier: int +) -> _PreparedQuery: + place, backwards = process_args(request.after, request.before, request.page) + + selectable = request.selectable + result_type = core_result_type(selectable, s) + sel = prepare_paging( + q=selectable, + per_page=request.per_page, + place=place, + backwards=backwards, + orm=False, + dialect=get_bind(q=selectable, s=s).dialect, + ) + def page_from_rows(rows, selected): + keys = list(selected.keys()) + N = len(keys) - len(sel.extra_columns) + keys = keys[:N] + page = core_page_from_rows( + sel, + rows, + keys, + result_type, + request.per_page, + backwards, + current_place=place, + ) + return page + + return _PreparedQuery(paging_query=sel, page_from_rows=page_from_rows) -def _prepare_homogeneous_page( - request: PageRequest[_TP], page_identifier: int +def _orm_prepare_homogeneous_page( + request: OrmPageRequest[_TP], page_identifier: int ) -> _PreparedQuery: place, backwards = process_args(request.after, request.before, request.page) diff --git a/tests/test_paging.py b/tests/test_paging.py index 5124d267..3ca1ece3 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -33,6 +33,7 @@ serialize_bookmark, unserialize_bookmark, InvalidPage, + OrmPageRequest, PageRequest, ) from sqlakeyset.paging import process_args @@ -113,7 +114,7 @@ def check_multiple_paging_orm(qs): t.page = unserialize_bookmark(serialize_bookmark(t.page)) page_requests = [ - PageRequest(query=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) + OrmPageRequest(query=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) ] pages_with_paging = get_homogeneous_pages(page_requests) for p, t in zip(pages_with_paging, page_trackers): @@ -157,6 +158,26 @@ def check_paging_orm(q): assert list(gathered) == unpaged +def assert_paging_core(page_with_paging, gathered, backwards, unpaged, page, per_page): + paging = page_with_paging.paging + + assert paging.current == page + assert page_with_paging.keys() == result.keys() + + if backwards: + gathered.extendleft(reversed(page_with_paging)) + else: + gathered.extend(page_with_paging) + + if not page_with_paging: + assert not paging.has_further + assert paging.further == paging.current + assert paging.current_opposite == (None, not paging.backwards) + return None + + return paging.further + + def check_paging_core(selectable, s): item_counts = range(1, 12) @@ -165,7 +186,7 @@ def check_paging_core(selectable, s): for backwards in [False, True]: for per_page in item_counts: - gathered = [] + gathered = deque() page = None, backwards @@ -176,22 +197,8 @@ def check_paging_core(selectable, s): page_with_paging = select_page( s, selectable, per_page=per_page, page=serialized_page ) - paging = page_with_paging.paging - - assert paging.current == page - assert page_with_paging.keys() == result.keys() - - if backwards: - gathered = page_with_paging + gathered - else: - gathered = gathered + page_with_paging - - page = paging.further - - if not page_with_paging: - assert not paging.has_further - assert paging.further == paging.current - assert paging.current_opposite == (None, not paging.backwards) + page = assert_paging_core(page_with_paging, gathered, backwards, unpaged, page, per_page) + if page is None: break assert gathered == unpaged From 1127c0f187dd8e75bd8c50c8301b3c44c710a996 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:17:33 -0700 Subject: [PATCH 35/64] flake8 fixes. --- sqlakeyset/__init__.py | 12 +++++++++++- sqlakeyset/paging.py | 11 +++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/sqlakeyset/__init__.py b/sqlakeyset/__init__.py index 4996f35e..47737356 100644 --- a/sqlakeyset/__init__.py +++ b/sqlakeyset/__init__.py @@ -1,4 +1,12 @@ -from .paging import get_homogeneous_pages, get_page, select_homogeneous_pages, select_page, InvalidPage, OrmPageRequest, PageRequest +from .paging import ( + get_homogeneous_pages, + get_page, + select_homogeneous_pages, + select_page, + InvalidPage, + OrmPageRequest, + PageRequest, +) from .results import ( Page, Paging, @@ -17,11 +25,13 @@ __all__ = [ "get_homogeneous_pages", "get_page", + "select_homogeneous_pages", "select_page", "serialize_bookmark", "unserialize_bookmark", "Page", "PageRequest", + "OrmPageRequest", "Paging", "Keyset", "Marker", diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 48da5c32..4e61e854 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -24,7 +24,7 @@ from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session from sqlalchemy.orm.query import Query -from sqlalchemy.sql.expression import ColumnElement, literal +from sqlalchemy.sql.expression import ColumnElement, literal, union_all from sqlalchemy.sql.selectable import Select from .columns import OC, MappedOrderColumn, find_order_key, parse_ob_clause @@ -537,10 +537,12 @@ def select_homogeneous_pages( prepared_queries = [_core_prepare_homogeneous_page(request, s, i) for i, request in enumerate(requests)] - select = union_all(*[p.paging_query.select for p in prepared_queries]) - selected = s.execute(sel.select) + select = union_all( + *[p.paging_query.select for p in prepared_queries] + ).order_by(text("_page_identifier"), text("_row_number")) + selected = s.execute(select) - results = query.fetchall() + results = selected.fetchall() # We need to make sure there's an entry for every page in case some return # empty. @@ -576,6 +578,7 @@ def _core_prepare_homogeneous_page( orm=False, dialect=get_bind(q=selectable, s=s).dialect, ) + def page_from_rows(rows, selected): keys = list(selected.keys()) N = len(keys) - len(sel.extra_columns) From 24943ea7789b304bc99059b68815c29841bd4926 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:20:27 -0700 Subject: [PATCH 36/64] More flake8. --- tests/test_paging.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 3ca1ece3..86667a0a 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -34,7 +34,6 @@ unserialize_bookmark, InvalidPage, OrmPageRequest, - PageRequest, ) from sqlakeyset.paging import process_args from sqlakeyset.results import Page @@ -158,7 +157,7 @@ def check_paging_orm(q): assert list(gathered) == unpaged -def assert_paging_core(page_with_paging, gathered, backwards, unpaged, page, per_page): +def assert_paging_core(page_with_paging, gathered, backwards, result, page, per_page): paging = page_with_paging.paging assert paging.current == page @@ -197,7 +196,7 @@ def check_paging_core(selectable, s): page_with_paging = select_page( s, selectable, per_page=per_page, page=serialized_page ) - page = assert_paging_core(page_with_paging, gathered, backwards, unpaged, page, per_page) + page = assert_paging_core(page_with_paging, gathered, backwards, result, page, per_page) if page is None: break From 5bb4c5d7d34d046fee36bbaca70d6ba305404363 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:23:12 -0700 Subject: [PATCH 37/64] Convert deque to list. --- tests/test_paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 86667a0a..340cc654 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -200,7 +200,7 @@ def check_paging_core(selectable, s): if page is None: break - assert gathered == unpaged + assert list(gathered) == unpaged def test_orm_query1(dburl): From 29985c410eaaeeabb2abf375f9bbe71c0f8171ed Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:34:48 -0700 Subject: [PATCH 38/64] Test select_homogeneous_pages. --- tests/test_paging.py | 123 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 97 insertions(+), 26 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 340cc654..8472381a 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -16,7 +16,7 @@ from collections import deque from dataclasses import dataclass from packaging import version -from typing import Optional, Tuple, Union +from typing import Any, Optional, Tuple, Union import pytest import sqlalchemy @@ -25,6 +25,7 @@ desc, func, ) +from sqlalchemy.sql.selectable import Select from sqlakeyset import ( get_page, @@ -58,8 +59,9 @@ @dataclass class _PageTracker: - query: Query + query: Union[Query, Select] unpaged: list + selected: Any gathered: deque backwards: bool page: Tuple[Union[MarkerLike, str], bool] @@ -97,6 +99,30 @@ def assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_ return paging.further +def check_paging_orm(q): + item_counts = range(1, 12) + + unpaged = q.all() + + for backwards in [False, True]: + for per_page in item_counts: + gathered = deque() + + page = None, backwards + + while True: + serialized_page = serialize_bookmark(page) + page = unserialize_bookmark(serialized_page) + + page_with_paging = get_page(q, per_page=per_page, page=serialized_page) + page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page) + if page is None: + break + + # Ensure union of pages is original q.all() + assert list(gathered) == unpaged + + def check_multiple_paging_orm(qs): page_trackers = [ _PageTracker( @@ -133,30 +159,6 @@ def check_multiple_paging_orm(qs): break -def check_paging_orm(q): - item_counts = range(1, 12) - - unpaged = q.all() - - for backwards in [False, True]: - for per_page in item_counts: - gathered = deque() - - page = None, backwards - - while True: - serialized_page = serialize_bookmark(page) - page = unserialize_bookmark(serialized_page) - - page_with_paging = get_page(q, per_page=per_page, page=serialized_page) - page = assert_paging_orm(page_with_paging, gathered, backwards, unpaged, page, per_page) - if page is None: - break - - # Ensure union of pages is original q.all() - assert list(gathered) == unpaged - - def assert_paging_core(page_with_paging, gathered, backwards, result, page, per_page): paging = page_with_paging.paging @@ -203,6 +205,43 @@ def check_paging_core(selectable, s): assert list(gathered) == unpaged +def check_multiple_paging_core(qs, s): + page_trackers = [ + _PageTracker( + query=q, + gathered=deque(), + backwards=(i % 2 == 0), + page=(None, i % 2 == 0), + selected=s.execute(q), + unpaged=s.execute(q).fetchall(), + ) + for i, q in enumerate(qs) + ] + while True: + for t in page_trackers: + t.page = unserialize_bookmark(serialize_bookmark(t.page)) + + page_requests = [ + PageRequest(selectable=t.query, per_page=i + 1, page=t.page) for i, t in enumerate(page_trackers) + ] + pages_with_paging = select_homogeneous_pages(page_requests, s) + for p, t in zip(pages_with_paging, page_trackers): + t.page_with_paging = p + + for i, t in enumerate(list(page_trackers)): + page = assert_paging_core(t.page_with_paging, t.gathered, t.backwards, t.selected, t.page, i + 1) + if page is None: + # Ensure union of pages is original q.all() + assert list(t.gathered) == t.unpaged + page_trackers.remove(t) + continue + + t.page = page + + if not page_trackers: + break + + def test_orm_query1(dburl): spec = [desc(Book.b), Book.d, Book.id] @@ -463,6 +502,38 @@ def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] +def test_core_multiple_pages(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: + qs = [ + select(Book).order_by(Book.name, Book.id), + select(Book).filter(Book.author_id == 1).order_by(Book.id), + select(Book).order_by(Book.name, Book.id.desc()), + ] + check_multiple_paging_core(qs=qs, s=s) + + +def test_core_multiple_pages_select_columns(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: + qs = [ + select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id), + select(Book.name, Book.author_id, Book.id).filter(Book.author_id == 1).order_by(Book.id), + select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), + ] + check_multiple_paging_core(qs=qs, s=s) + + +def test_core_multiple_pages_one_query(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: + qs = [ + select(Book).order_by(Book.id), + ] + check_multiple_paging_core(qs=qs, s=s) + + +def test_core_multiple_pages_empty_queries(): + assert select_homogeneous_pages([]) == [] + + def test_core(dburl): selectable = ( select(Book.b, Book.d, Book.id, Book.c) From 87a6a08dfcf57564522aa4243be5486c3d192959 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:41:55 -0700 Subject: [PATCH 39/64] Flake 8 violations. --- tests/test_paging.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 8472381a..3b0393e4 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -28,13 +28,15 @@ from sqlalchemy.sql.selectable import Select from sqlakeyset import ( - get_page, get_homogeneous_pages, + get_page, + select_homogeneous_pages, select_page, serialize_bookmark, unserialize_bookmark, InvalidPage, OrmPageRequest, + PageRequest, ) from sqlakeyset.paging import process_args from sqlakeyset.results import Page From 993e45fd0b21e3ed790e7dadac9d171e8a73a42b Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:50:02 -0700 Subject: [PATCH 40/64] Fix default. --- tests/test_paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 3b0393e4..f921cf30 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -63,7 +63,7 @@ class _PageTracker: query: Union[Query, Select] unpaged: list - selected: Any + selected: Any = None gathered: deque backwards: bool page: Tuple[Union[MarkerLike, str], bool] From 23ec6da81d96f23a51761f219d48246cf9e1b510 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:51:11 -0700 Subject: [PATCH 41/64] non-default before default. --- tests/test_paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index f921cf30..44201593 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -63,10 +63,10 @@ class _PageTracker: query: Union[Query, Select] unpaged: list - selected: Any = None gathered: deque backwards: bool page: Tuple[Union[MarkerLike, str], bool] + selected: Any = None page_with_paging: Optional[Page] = None From aff47d87f58ca44a8d06955a1798f912db7e68e9 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 11:53:44 -0700 Subject: [PATCH 42/64] Add page_identifier --- sqlakeyset/paging.py | 1 + 1 file changed, 1 insertion(+) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 4e61e854..2f61b493 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -577,6 +577,7 @@ def _core_prepare_homogeneous_page( backwards=backwards, orm=False, dialect=get_bind(q=selectable, s=s).dialect, + page_identifier=page_identifier, ) def page_from_rows(rows, selected): From dfee9ddee901fa13fdc326c2bdf3e7cec4527c5b Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 12:11:01 -0700 Subject: [PATCH 43/64] filter -> where. --- sqlakeyset/paging.py | 2 ++ tests/test_paging.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 2f61b493..975741f9 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -218,6 +218,7 @@ def prepare_paging( order_by=[c.uo for c in order_cols] ).label("_row_number"), ] + if hasattr(q, "add_columns"): # ORM or SQLAlchemy 1.4+ q = q.add_columns(*extra_columns) else: @@ -582,6 +583,7 @@ def _core_prepare_homogeneous_page( def page_from_rows(rows, selected): keys = list(selected.keys()) + print(keys) N = len(keys) - len(sel.extra_columns) keys = keys[:N] page = core_page_from_rows( diff --git a/tests/test_paging.py b/tests/test_paging.py index 44201593..bbb4fc11 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -508,7 +508,7 @@ def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ select(Book).order_by(Book.name, Book.id), - select(Book).filter(Book.author_id == 1).order_by(Book.id), + select(Book).where(Book.author_id == 1).order_by(Book.id), select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) @@ -518,7 +518,7 @@ def test_core_multiple_pages_select_columns(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id), - select(Book.name, Book.author_id, Book.id).filter(Book.author_id == 1).order_by(Book.id), + select(Book.name, Book.author_id, Book.id).where(Book.author_id == 1).order_by(Book.id), select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) From 403a3977399be7cfc1ab693fd897ad699fbbe7fc Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 12:14:10 -0700 Subject: [PATCH 44/64] Add debugging. --- sqlakeyset/paging.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 975741f9..0138e212 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -583,9 +583,10 @@ def _core_prepare_homogeneous_page( def page_from_rows(rows, selected): keys = list(selected.keys()) - print(keys) N = len(keys) - len(sel.extra_columns) keys = keys[:N] + print(keys) + print(rows[0]) page = core_page_from_rows( sel, rows, From ead71a30c1e0200448f11bccd7ac2368ce2115f0 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 12:17:09 -0700 Subject: [PATCH 45/64] Fix select statements. --- tests/test_paging.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index bbb4fc11..24c1db43 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -507,9 +507,9 @@ def test_orm_multiple_pages_empty_queries(): def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ - select(Book).order_by(Book.name, Book.id), - select(Book).where(Book.author_id == 1).order_by(Book.id), - select(Book).order_by(Book.name, Book.id.desc()), + select(Book.name).order_by(Book.name, Book.id), + select(Book.name).where(Book.author_id == 1).order_by(Book.id), + select(Book.name).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) @@ -527,7 +527,7 @@ def test_core_multiple_pages_select_columns(no_sqlite_dburl): def test_core_multiple_pages_one_query(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ - select(Book).order_by(Book.id), + select(Book.a, Book.b).order_by(Book.id), ] check_multiple_paging_core(qs=qs, s=s) From 685fca4bc2d3f08b0b7ce6136f6da8418dbe81f8 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Fri, 14 Jul 2023 12:25:09 -0700 Subject: [PATCH 46/64] Go back to Book orm. --- tests/test_paging.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 24c1db43..2701b741 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -507,9 +507,9 @@ def test_orm_multiple_pages_empty_queries(): def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ - select(Book.name).order_by(Book.name, Book.id), - select(Book.name).where(Book.author_id == 1).order_by(Book.id), - select(Book.name).order_by(Book.name, Book.id.desc()), + select(Book).order_by(Book.name, Book.id), + select(Book).where(Book.author_id == 1).order_by(Book.id), + select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) From ccb3e915ac214827fbe7124c650de5a3f3652266 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 11:20:39 -0700 Subject: [PATCH 47/64] add a print statement. --- sqlakeyset/paging.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 0138e212..41ebf1a9 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -583,9 +583,10 @@ def _core_prepare_homogeneous_page( def page_from_rows(rows, selected): keys = list(selected.keys()) + print(f"pre-shrunk keys: {keys}") N = len(keys) - len(sel.extra_columns) keys = keys[:N] - print(keys) + print(f"post-shrunk keys: {keys}") print(rows[0]) page = core_page_from_rows( sel, From 4f62fba2bbc0c8ef3933e04569bd8bec8a3b1053 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 11:34:07 -0700 Subject: [PATCH 48/64] Start with the simplest test first. --- tests/test_paging.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 2701b741..a17045ea 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -503,7 +503,7 @@ def test_orm_multiple_pages_one_query(no_sqlite_dburl): def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] - +""" def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ @@ -522,7 +522,7 @@ def test_core_multiple_pages_select_columns(no_sqlite_dburl): select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) - +""" def test_core_multiple_pages_one_query(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: From 43694f09ab775a33f2de3c2c901cb0d67dc13c30 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 11:37:15 -0700 Subject: [PATCH 49/64] flake8 --- tests/test_paging.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_paging.py b/tests/test_paging.py index a17045ea..4202d873 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -503,6 +503,7 @@ def test_orm_multiple_pages_one_query(no_sqlite_dburl): def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] + """ def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: @@ -524,6 +525,7 @@ def test_core_multiple_pages_select_columns(no_sqlite_dburl): check_multiple_paging_core(qs=qs, s=s) """ + def test_core_multiple_pages_one_query(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ From 34c41b23dde8fa9a94c9fe2862f0ea6511c6a465 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 11:41:28 -0700 Subject: [PATCH 50/64] Print statement caused exception. --- sqlakeyset/paging.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 41ebf1a9..b32d66de 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -587,7 +587,8 @@ def page_from_rows(rows, selected): N = len(keys) - len(sel.extra_columns) keys = keys[:N] print(f"post-shrunk keys: {keys}") - print(rows[0]) + if rows: + print(rows[0]) page = core_page_from_rows( sel, rows, From db47749535190581bc6886d10a10f7aa5d4e1ded Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 12:55:45 -0700 Subject: [PATCH 51/64] Don't double order single selects. --- sqlakeyset/paging.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index b32d66de..2ab8f824 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -540,7 +540,10 @@ def select_homogeneous_pages( select = union_all( *[p.paging_query.select for p in prepared_queries] - ).order_by(text("_page_identifier"), text("_row_number")) + ) + if len(prepared_queries) > 1: + select = select.order_by(text("_page_identifier"), text("_row_number")) + selected = s.execute(select) results = selected.fetchall() From 90e9ae760eb03b3944de40fda681f958e93ae6c7 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:00:36 -0700 Subject: [PATCH 52/64] Fix test_core..._empty_queries. --- tests/test_paging.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 4202d873..6a054ba3 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -535,7 +535,8 @@ def test_core_multiple_pages_one_query(no_sqlite_dburl): def test_core_multiple_pages_empty_queries(): - assert select_homogeneous_pages([]) == [] + with S(no_sqlite_dburl, echo=ECHO) as s: + assert select_homogeneous_pages([], s) == [] def test_core(dburl): From 8ecb334a6aba9b525f6a0a4b6f379b8ebadc751f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:02:12 -0700 Subject: [PATCH 53/64] flake8. --- tests/test_paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index 6a054ba3..dd0e6d11 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -534,7 +534,7 @@ def test_core_multiple_pages_one_query(no_sqlite_dburl): check_multiple_paging_core(qs=qs, s=s) -def test_core_multiple_pages_empty_queries(): +def test_core_multiple_pages_empty_queries(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: assert select_homogeneous_pages([], s) == [] From 6c33a0f3e3258092dbcfe869ebc0adb198458c2e Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:05:14 -0700 Subject: [PATCH 54/64] One more test. --- sqlakeyset/paging.py | 1 + tests/test_paging.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 2ab8f824..edbea85d 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -544,6 +544,7 @@ def select_homogeneous_pages( if len(prepared_queries) > 1: select = select.order_by(text("_page_identifier"), text("_row_number")) + print(f"Select statement: {select}") selected = s.execute(select) results = selected.fetchall() diff --git a/tests/test_paging.py b/tests/test_paging.py index dd0e6d11..eab245a4 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -513,6 +513,7 @@ def test_core_multiple_pages(no_sqlite_dburl): select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) +""" def test_core_multiple_pages_select_columns(no_sqlite_dburl): @@ -523,7 +524,6 @@ def test_core_multiple_pages_select_columns(no_sqlite_dburl): select(Book.name, Book.author_id, Book.id).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) -""" def test_core_multiple_pages_one_query(no_sqlite_dburl): From e230d1a421ea637de237551f4a2e5e036518dac4 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:19:08 -0700 Subject: [PATCH 55/64] Try using sub-select for key generation. --- sqlakeyset/paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index edbea85d..575cba86 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -558,7 +558,7 @@ def select_homogeneous_pages( pages = [] for i in range(len(requests)): rows = page_to_rows[i] - pages.append(prepared_queries[i].page_from_rows(rows, selected)) + pages.append(prepared_queries[i].page_from_rows(rows, s.execute(prepared_queries[i].paging_query.select))) return pages From 0917ceb340999d0904cc28d1bd5a38be4c4da906 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:24:54 -0700 Subject: [PATCH 56/64] See if individualized selected works for select(Book) --- tests/test_paging.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_paging.py b/tests/test_paging.py index eab245a4..b3b93c2e 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -504,7 +504,6 @@ def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] -""" def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ @@ -513,7 +512,6 @@ def test_core_multiple_pages(no_sqlite_dburl): select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) -""" def test_core_multiple_pages_select_columns(no_sqlite_dburl): From 719440c222787852d67a4119bf25244bf6d8674f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:40:24 -0700 Subject: [PATCH 57/64] Try a different way of forming the union. --- sqlakeyset/paging.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 575cba86..d5318667 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -538,10 +538,14 @@ def select_homogeneous_pages( prepared_queries = [_core_prepare_homogeneous_page(request, s, i) for i, request in enumerate(requests)] + """ select = union_all( *[p.paging_query.select for p in prepared_queries] ) + """ + select = prepared_queries[0].paging_query.select if len(prepared_queries) > 1: + select = select.union_all(*[p.paging_query.select for p in prepared_queries[1:]]) select = select.order_by(text("_page_identifier"), text("_row_number")) print(f"Select statement: {select}") From e192516bfb68a08035439fc27d640c38fe89047e Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:41:38 -0700 Subject: [PATCH 58/64] flake8 --- sqlakeyset/paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index d5318667..a8a2330f 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -24,7 +24,7 @@ from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session from sqlalchemy.orm.query import Query -from sqlalchemy.sql.expression import ColumnElement, literal, union_all +from sqlalchemy.sql.expression import ColumnElement, literal from sqlalchemy.sql.selectable import Select from .columns import OC, MappedOrderColumn, find_order_key, parse_ob_clause From 6b596a5fe79f27745df2f0076c2fe41c57ef990f Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:44:32 -0700 Subject: [PATCH 59/64] Backtrack on testing, plus add a more fundamental test. --- tests/test_paging.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_paging.py b/tests/test_paging.py index b3b93c2e..f37a91ab 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -504,6 +504,7 @@ def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] +""" def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ @@ -512,6 +513,7 @@ def test_core_multiple_pages(no_sqlite_dburl): select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) +""" def test_core_multiple_pages_select_columns(no_sqlite_dburl): @@ -552,6 +554,17 @@ def test_core(dburl): check_paging_core(selectable=selectable, s=s.connection()) +def test_core_whole_model(dburl): + selectable = ( + select(Book) + .where(Book.d == 99) + .order_by(Book.id) + ) + + with S(dburl, echo=ECHO) as s: + check_paging_core(selectable=selectable, s=s) + + def test_core2(dburl): with S(dburl, echo=ECHO) as s: sel = select(Book.score).order_by(Book.id) From 806ef9d2a6178db7562a9358a6e74bc5e2bc2bc0 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:47:46 -0700 Subject: [PATCH 60/64] Add a test. --- tests/test_paging.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_paging.py b/tests/test_paging.py index f37a91ab..e206a71e 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -534,6 +534,14 @@ def test_core_multiple_pages_one_query(no_sqlite_dburl): check_multiple_paging_core(qs=qs, s=s) +def test_core_multiple_pages_one_query_whole_model(no_sqlite_dburl): + with S(no_sqlite_dburl, echo=ECHO) as s: + qs = [ + select(Book).order_by(Book.id), + ] + check_multiple_paging_core(qs=qs, s=s) + + def test_core_multiple_pages_empty_queries(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: assert select_homogeneous_pages([], s) == [] From fa5fd494817f9dfed9a6ab5a936d7c599c09d142 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 13:55:14 -0700 Subject: [PATCH 61/64] Only execute one subselect as necessary. --- sqlakeyset/paging.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index a8a2330f..b3dddb65 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -538,14 +538,10 @@ def select_homogeneous_pages( prepared_queries = [_core_prepare_homogeneous_page(request, s, i) for i, request in enumerate(requests)] - """ select = union_all( *[p.paging_query.select for p in prepared_queries] ) - """ - select = prepared_queries[0].paging_query.select if len(prepared_queries) > 1: - select = select.union_all(*[p.paging_query.select for p in prepared_queries[1:]]) select = select.order_by(text("_page_identifier"), text("_row_number")) print(f"Select statement: {select}") @@ -560,9 +556,17 @@ def select_homogeneous_pages( page_to_rows[row._page_identifier].append(row) pages = [] + # This is an unfortunate side effect of union_all. It appears we union_all + # a bunch of selects, it changes the "keys" on us in cases where the column + # name and python attribute don't match. So we have to execute the first + # query standalone to ge the correct keys. + subselect_result = ( + s.execute(prepared_queries[0].paging_query.select) + if len(prepared_queries) > 1 else selected + ) for i in range(len(requests)): rows = page_to_rows[i] - pages.append(prepared_queries[i].page_from_rows(rows, s.execute(prepared_queries[i].paging_query.select))) + pages.append(prepared_queries[i].page_from_rows(rows, subselect_result)) return pages From 1b9133b767ecba64fb733d27e6669c756dd49597 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 14:02:03 -0700 Subject: [PATCH 62/64] flake8 --- sqlakeyset/paging.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index b3dddb65..4313ed07 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -24,7 +24,7 @@ from sqlalchemy.engine.interfaces import Dialect from sqlalchemy.orm import Session from sqlalchemy.orm.query import Query -from sqlalchemy.sql.expression import ColumnElement, literal +from sqlalchemy.sql.expression import ColumnElement, literal, union_all from sqlalchemy.sql.selectable import Select from .columns import OC, MappedOrderColumn, find_order_key, parse_ob_clause From 1909b8e9e29d25896f6742b242f8a384976d5f72 Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 14:11:40 -0700 Subject: [PATCH 63/64] Clean up single query select. --- sqlakeyset/paging.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index 4313ed07..d5d77043 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -538,10 +538,12 @@ def select_homogeneous_pages( prepared_queries = [_core_prepare_homogeneous_page(request, s, i) for i, request in enumerate(requests)] - select = union_all( - *[p.paging_query.select for p in prepared_queries] - ) - if len(prepared_queries) > 1: + if len(requests) == 1: + select = prepared_queries[0].paging_query.select + else: + select = union_all( + *[p.paging_query.select for p in prepared_queries] + ) select = select.order_by(text("_page_identifier"), text("_row_number")) print(f"Select statement: {select}") From 1af24f8d8c57121b6d0280ed4d22894098c20feb Mon Sep 17 00:00:00 2001 From: Matthew Albrecht Date: Tue, 18 Jul 2023 14:38:34 -0700 Subject: [PATCH 64/64] Uncomment test to send Anthony error. --- sqlakeyset/paging.py | 2 +- tests/test_paging.py | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/sqlakeyset/paging.py b/sqlakeyset/paging.py index d5d77043..85ba6ab8 100644 --- a/sqlakeyset/paging.py +++ b/sqlakeyset/paging.py @@ -558,7 +558,7 @@ def select_homogeneous_pages( page_to_rows[row._page_identifier].append(row) pages = [] - # This is an unfortunate side effect of union_all. It appears we union_all + # This is an unfortunate side effect of union_all. It appears when we union_all # a bunch of selects, it changes the "keys" on us in cases where the column # name and python attribute don't match. So we have to execute the first # query standalone to ge the correct keys. diff --git a/tests/test_paging.py b/tests/test_paging.py index e206a71e..85c3ecd9 100644 --- a/tests/test_paging.py +++ b/tests/test_paging.py @@ -504,7 +504,6 @@ def test_orm_multiple_pages_empty_queries(): assert get_homogeneous_pages([]) == [] -""" def test_core_multiple_pages(no_sqlite_dburl): with S(no_sqlite_dburl, echo=ECHO) as s: qs = [ @@ -513,7 +512,6 @@ def test_core_multiple_pages(no_sqlite_dburl): select(Book).order_by(Book.name, Book.id.desc()), ] check_multiple_paging_core(qs=qs, s=s) -""" def test_core_multiple_pages_select_columns(no_sqlite_dburl):