Impelement get_homogeneous_pages. by mattalbr · Pull Request #87 · djrobstep/sqlakeyset

mattalbr · 2023-07-11T19:02:15Z

Unit tests forthcoming once we're directionally aligned.

mattalbr · 2023-07-11T19:02:37Z

acarapetis · 2023-07-11T23:50:10Z

sqlakeyset/paging.py

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


missing a return pages?

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

acarapetis · 2023-07-11T23:52:08Z

sqlakeyset/paging.py

+
+    # 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)


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

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

acarapetis · 2023-07-11T23:54:46Z

sqlakeyset/paging.py

+    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")


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

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

acarapetis · 2023-07-11T23:56:50Z

sqlakeyset/paging.py

+    # 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(


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

acarapetis · 2023-07-11T23:58:43Z

sqlakeyset/paging.py

    return orm_get_page(query, per_page, place, backwards)
+
+
+# Do we need to support python 3.6?


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

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

acarapetis · 2023-07-12T00:02:34Z

sqlakeyset/paging.py

@@ -3,6 +3,7 @@
 from __future__ import annotations



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

acarapetis · 2023-07-12T00:03:21Z

sqlakeyset/paging.py

+    page: Optional[Union[MarkerLike, str]] = None
+
+
+def get_homogeneous_page(queries: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]:


I definitely think it should be get_homogeneous_pages.

Whoops, typo!

acarapetis · 2023-07-12T00:40:28Z

I definitely think some version of this is worth including, since it's not achievable using our current documented public API; so thanks!

I've added a bunch of minor comments in my review above, should be easy fixes. I do have a couple more significant concerns:

Since the Query API is now considered legacy, I definitely think we want an equivalent select_homogeneous_pages API to work with Select.
Unless I'm mistaken, the _page_identifier column is going to be hanging around on the rows returned to the user, which is not desirable. We already handle this same issue for added keyset columns in orm_page_from_rows, so hopefully we just need to add an additional item to paging_query.extra_columns - let me know if you need help with this.

And yeah, bring on the tests. :)

mattalbr

I'm almost there, just still struggling with select_homogeneous_pages. It keeps failing trying to grab the keys. Let me know if you see something obvious, otherwise I'll keep fighting with it.

Otherwise, good for you to take another look.

mattalbr · 2023-07-12T15:35:28Z

sqlakeyset/paging.py

    return orm_get_page(query, per_page, place, backwards)
+
+
+# Do we need to support python 3.6?


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

mattalbr · 2023-07-12T15:35:50Z

sqlakeyset/paging.py

+    page: Optional[Union[MarkerLike, str]] = None
+
+
+def get_homogeneous_page(queries: list[PageRequest[_TP]]) -> list[Page[Row[_TP]]]:


Whoops, typo!

mattalbr · 2023-07-12T15:37:51Z

sqlakeyset/paging.py

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


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

mattalbr · 2023-07-12T15:39:07Z

sqlakeyset/paging.py

+
+    # 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)


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

mattalbr · 2023-07-12T15:39:26Z

sqlakeyset/paging.py

+    # 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(


mattalbr · 2023-07-12T15:49:41Z

sqlakeyset/paging.py

+    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")


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

mattalbr · 2023-07-20T23:11:13Z

Closing in favor of #90 which I finally got working for both ORM and select.

Impelement get_homogeneous_pages.

ff21c99

Unit tests forthcoming once we're directionally aligned.

acarapetis requested changes Jul 12, 2023

View reviewed changes

mattalbr added 26 commits July 13, 2023 08:30

Check in unit test for multi-page lookup.

eff5d52

Fix missing symbols.

d37132b

Fix flake8 test_paging.py

df7466c

Export get_homogeneous_pages.

4843e68

Actually import get_homogeneous_pages.

9985e4e

Add PageRequest.

bd486e1

Fix bad symbol.

e534243

Extend gathered.

5e6fd6a

Use Union instead of |.

f5075c3

Use Tuple instead of tuple.

9aae97f

Use Book instead of Animal.

cbefa30

More python 3.7 and fix wrong symbol name.

bb86b7e

Improve tests.

cdd0fc2

Remove unused db session.

2af1ca2

Exclude sqlite.

76c2939

s/dburl/no_sqlite_url

6867d83

s/url/dburl

19ff009

Fix for loop variable.

abdef8a

Per page should be at least 1.

a5514cf

Fix UNION ALL ordering which isn't guaranteed.

b7a34f5

NamedTuple is immutable.

2f4f4bd

Trailing whitespace.

3909554

Use uo instead of element to get ordering right for ROW_NUMBER.

3393641

Use deque for extendleft functionality.

f985063

Debugging info.

e5feac2

Testing.

46248d2

mattalbr added 8 commits July 14, 2023 11:41

Flake 8 violations.

87a6a08

Fix default.

993e45f

non-default before default.

23ec6da

Add page_identifier

aff47d8

filter -> where.

dfee9dd

Add debugging.

403a397

Fix select statements.

ead71a3

Go back to Book orm.

685fca4

mattalbr commented Jul 14, 2023

View reviewed changes

mattalbr requested a review from acarapetis July 18, 2023 17:48

mattalbr added 18 commits July 18, 2023 11:20

add a print statement.

ccb3e91

Start with the simplest test first.

4f62fba

flake8

43694f0

Print statement caused exception.

34c41b2

Don't double order single selects.

db47749

Fix test_core..._empty_queries.

90e9ae7

flake8.

8ecb334

One more test.

6c33a0f

Try using sub-select for key generation.

e230d1a

See if individualized selected works for select(Book)

0917ceb

Try a different way of forming the union.

719440c

flake8

e192516

Backtrack on testing, plus add a more fundamental test.

6b596a5

Add a test.

806ef9d

Only execute one subselect as necessary.

fa5fd49

flake8

1b9133b

Clean up single query select.

1909b8e

Uncomment test to send Anthony error.

1af24f8

mattalbr closed this Jul 20, 2023

		return orm_get_page(query, per_page, place, backwards)


		# Do we need to support python 3.6?

		page: Optional[Union[MarkerLike, str]] = None


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

Conversation

mattalbr commented Jul 11, 2023

Uh oh!

mattalbr commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acarapetis commented Jul 12, 2023

Uh oh!

mattalbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattalbr commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants