Skip to content

Add typed select() for column selection - #89

Merged
davegaeddert merged 68 commits into
masterfrom
select
Sep 20, 2026
Merged

davegaeddert merged 68 commits into
masterfrom
select

Conversation

@davegaeddert

@davegaeddert davegaeddert commented Jul 23, 2026

Copy link
Copy Markdown
Member

What

Adds QuerySet.select(*items, flat=False, result_type=None) — typed column selection that returns honest rows (tuples, flat scalars, or dataclass instances) via a new RowQuerySet[R], never partial model instances.

User.query.where(...).select(User.id, User.email)       # RowQuerySet[tuple[int, str]]
User.query.select(User.email, flat=True)                # RowQuerySet[str]
User.query.select(User.id, User.email, result_type=UserStats)  # RowQuerySet[UserStats]

Stacked on typed-where (#84), which provides the typed field-access surface.

Vocabulary decision

select() and where() are the primary query API from here on. The string-based forms — values(), values_list(), only(), defer(), annotate(), filter()remain in place and are untouched by this PR. They get swept in a follow-up slice; that sweep is deferred deliberately, not forgotten, and nothing here deprecates or removes them.

The sweep is set up to be a migration rather than a rewrite: select() is built on top of the existing _values_list plumbing (same _valuesset_values path, same converters, same iterables), so the two produce identical SQL and identical row values. The one thing that can't invert is direction — values_list() accepts bare strings, which select() deliberately rejects — so consumers move to select() and then values_list() goes away, not the other way round.

Design

  • Selectable[T] baseField[T] and F/BaseExpression subclass a shared generic marker so select() can bind each column's value type. It's a bare class Selectable[T]: pass; a checker solves T from Field[str] <: Selectable[T] by specializing the base class, so the marker needs no members.
  • Rows, not instances — everything that reads rows is inherited from QuerySet. RowQuerySet adds only the refusals and, under TYPE_CHECKING, the R the base can't carry.
  • Fields resolve by name; expressions get internal auto-aliases via the existing values_list plumbing. result_type maps columns onto dataclass fields positionally (arity + field-name checks at select() time).
  • Row mode is row mode, whoever entered itupdate() now carries the same _fields is not None guard delete() already had, so values(), values_list() and select() all refuse writes. Previously values("name").update(...) silently ran.
  • select() goes last in a chain. Anything that would change the SELECT list afterwards is refused: values(), values_list(), only(), defer() and select_related() already raised, and annotate() now joins them (annotate first, then select()). Re-selecting is last-wins for the column list — expression columns included, which take the internal annotate path rather than the guarded public one. Joins introduced by an earlier expression persist, so select(Upper("tags__name")).select(Widget.name) still returns one row per joined row and count()/exists() count those. That is annotate(...) + values_list(...) behaving as it always has; trimming joins no queryset needs any more is out of scope here, and noted in the README.
  • A column belongs to the model whose field built it. Order.query.select(User.email) raises TypeError naming both models — the select() half of the guard #133 gave where(), reusing the Field.source_model it added. The check runs ahead of the traversal refusal, so a traversed column rooted on another model is reported as the cross-model mistake it is. Expressions are unaffected — F("email") takes a string resolved against whatever query it lands in, like filter()'s kwargs.
  • prefetch_related() is refused in both orders. A prefetch attaches related objects to a model instance's attributes, and a row has nowhere to put them. Select the columns you need from the related model instead.
  • result_type columns map onto the constructor, read off inspect.signature() rather than dataclasses.fields() — the two disagree in both directions, since an InitVar is a constructor parameter that never appears in fields() and an init=False field appears there but can't be passed. Each value is passed by the kind its parameter takes: positional-only and positional-or-keyword by position, keyword-only by name.
  • Chaining methods are annotated Self, not QuerySet[T] — otherwise RowQuerySet[R], which specializes its base as QuerySet[Any], drops R through reverse(), none(), order_by() and friends. That fix lives in the base QuerySet and also stops a custom queryset subclass being erased mid-chain.

Typing

The overload ladder is keyed on Selectable[T] itself. Pure-field selects get precise per-column types; a select that mixes in an expression types that expression's column as Any while the fields around it stay precise — e.g. select(D.priority, Upper("name"))RowQuerySet[tuple[int, Any]].

A column annotated Field[Any] is rejected by select(): Any satisfies the model-valued __get__ overload, so class access resolves as type[Any] rather than a field and no overload matches. That is the same reason the field-annotation guidance says never to annotate a field Field[Any] — use the concrete type, or Field[object] when the column really does hold arbitrary JSON, which select() types as object. No code change; noted here and in the README because the diagnostic (a fourteen-overload no-matching-overload dump) doesn't explain itself.

The ladder runs to ten columns; an eleventh still works but degrades to tuple[Any, ...]. All ten rungs are hand-written with no codegen, so tests/typing/select_ladder.py asserts every one of them against a model with ten distinctly typed columns — a transposed typevar in a middle rung can't ship silently. (Verified the hard way: transposing two typevars in the six- and eight-column rungs each fires a type-assertion-failure now, and passed clean before the file existed.)

Not in this slice

Relations are not selectable. select(Post.author), select(Post.author.city) and select(Post.author.id) all raise TypeError. The reason is nullability, not effort: a column reached through a relation arrives over a join, so a nullable relation yields None where the traversed field's type says int/str. That includes the foreign key's own column. values_list("author__id", flat=True) is the spelling until select() can express it, and the error message names it.

A custom QuerySet subclass is not preservedselect() hands back a plain RowQuerySet, so chain your own methods before select(), not after. Documented in the README.

Known gaps (follow-up, not introduced here)

  • Traversed columns type-check but fail at runtime. Post.author.id is Field[int] to a checker whether it was traversed or not, so the runtime refusal is the only guard. Closing it statically needs Selectable to carry model identity.
  • result_type mode allocates one tuple per row that it immediately unpacks and discards (tuple_expected=True in the shared values_list path).

Tests / checks

./scripts/fix, ./scripts/type-check . and the full ./scripts/test are green — 27 packages, 0 failures, 1371 passed / 5 skipped in plain-postgres (88 in test_select.py).

Rebased on master after #133 merged; the merge also gave master's four lock methods (for_update() and friends, from #87) the same Self treatment as the rest of the chaining surface, so rows.for_update() keeps its row type.

(One plain-dev test, test_nested_app_directories_do_not_collide_across_worktrees, fails in my environment because it shells out to git commit and the signing agent is locked. It reproduces on pristine master with no changes, so it is environmental and unrelated.)

Every column kind was exercised against a live database: encrypted (decrypts), JSON, Decimal/UUID/date/time/duration/bool/choices, timezone-aware datetimes (all converters correct — the values_list path is reused wholesale), plus distinct(), order_by on an unselected column, annotate before select, filter/where after, slicing, get() with 0/2 rows, first() on empty, exists(), count(), iterator(), pickling, and select() twice.

Review follow-ups

Both Codex findings are fixed and their threads resolved:

  • P1annotate() after select() appended a column under the declared row type: silently for tuples (RowQuerySet[tuple[str]] yielding ("alpha", 1)), as a confusing constructor error for result_type=, and silently dropped for flat=. Refused now, naming the supported order. 0bb3f36523
  • P2result_type arity was read off dataclasses.fields(), which omits InitVar constructor parameters and includes init=False fields. Now read off inspect.signature(). 86feed2111

Round two:

  • A result_type constructor mixing positional-only and keyword-only parameters (__init__(self, name, /, *, priority)) validated and then failed, because every value was passed by keyword. The row now splits by parameter kind at a single point computed once per query. a4474208c1
  • Inherited methods annotated -> QuerySet[T] dropped the row type on RowQuerySet[R] (and erased custom queryset subclasses generally). Swept to Self in the base. 5ce62e9400

Round three:

  • select() twice with an expression (select(D.name).select(F("priority"))) raised Cannot call annotate() after select(), because _values_list aliases expression columns by calling the public annotate(), which RowQuerySet refuses. The mechanism moved to _annotate(); the caller-facing guard is unchanged. fc6c97d5f6
  • prefetch_related("tags").select(..., result_type=NameRow) raised AttributeError: Cannot find 'tags' on NameRow object at iteration — and in tuple/flat mode didn't raise at all, so the prefetch query ran and its results went nowhere. Refused in both orders now, like select_related(). 7e4b60e0c0

Round four (live-DB review):

  • P1select() twice crashed when both selections held an expression (select(Upper("name")).select(Upper("status"))ValueError: The annotation 'upper1' conflicts with a field on the model). The internal alias counter restarted on every select() and only avoided the columns being selected right then, so it regenerated the previous alias; the message named neither the cause nor a real field. 290fb8ae69
  • P2, same root, no error at all — an internal alias could land on a name the caller had already used: annotate(upper1=Lower("name")).order_by("upper1").select(D.name, Upper("status")) came back ordered by UPPER(status). The generator now clears existing annotations and previously selected columns too. 290fb8ae69
  • P5model_qs | row_qs and model_qs & row_qs raised RecursionError, because _merge_sanity_check only inspected the left operand. Symmetric now. Pre-existing via values()/values_list(); select() is just the first typed door to it. a5e7350eb8
  • P3/P4get_or_none() returned Any | None instead of R | None; create() and bulk_create() were the two writes not refused on a RowQuerySet. 260a88aea8

__or__ used to be the one method that couldn't be Self — a sliced left operand is re-expressed as an id subquery against Meta.base_queryset, which is a plain QuerySet by design (it must never be a user-defined queryset, which might filter rows out). #85 settled it with an overload pair and a cast on that branch, so after merging master the row type survives | like everywhere else, and the corpus claim that recorded the degradation now records that it holds.

Review round five

Five non-blocking items from a live-DB review, all in 6383297030:

  • A field read off a mixin class raised a bare AssertionError; it's a TypeError naming the mistake now. (A checker rejects the access too, so this is the backstop for an untyped call site.)
  • A sliced RowQuerySet in | hit select()'s own values() refusal through the id-subquery fallback; the internal paths use _values().
  • _merge_sanity_check compared columns but not row shape, so a result_type= queryset merged with a tuple one and quietly returned the left operand's shape. It compares _iterable_class and _select_result_type too, with its own message.
  • The expression alias generator now steps over the model's own column names, so a model with a column called upper1 or f1 can't be shadowed. AliasCollisionExample exists to prove it.
  • The README's lead select() example didn't type-check (email: str rather than Field[str], plus an unnecessary query: line). Rewritten, and the same example is in the typing corpus so it can't rot.

Then merged master for #85's returning() (006b751070), and again for #86's bulk_upsert() and #88's upsert() (63c0df464d). That last merge is where the write stack ends, so this branch is now the only one outstanding.

The upsert merge needed two corrections a structural diff against master caught: RowQuerySet.update_or_create() was overriding a method #88 deleted, so it refused an API that no longer exists; and upsert()/bulk_upsert() were new writes RowQuerySet didn't refuse, so both ran on a row-mode queryset and handed back model instances. Both are settled, with tests.

Merge-verification follow-ups

  • bulk_update() already failed on a row-mode queryset, but from the update() inside its own transaction.atomic(savepoint=False), which left the enclosing transaction unusable — the next query raised TransactionManagementError. It refuses up front now, with nothing sent. 3e28b260da
  • returning() was accepted and inert on a RowQuerySet, and statically worse than inert: the checker went on believing update() hands back instances. Both orders refused. 3e28b260da
  • The ladder is now asserted rung by rung. 46d158af04

First slice of the typed query API. Field classes are now generic over
their value type T; stubs return the parameterized descriptor instead of
the primitive. Combined with Field's overloaded __get__, this gives:

    class User(postgres.Model):
        email = types.EmailField()
        note = types.TextField(allow_null=True, required=False)

    User.email   # EmailField[str]         — typed reference, .equals()/.contains()/...
    user.email   # str                     — value type preserved
    User.note    # TextField[str | None]
    user.note    # str | None              — nullability preserved

Adds equals/not_equal/gt/gte/lt/lte/is_null on Field[T] and
contains/icontains/startswith/endswith on TextField, each returning Q.
New QuerySet.where(*conditions: Q) accepts them positionally with no
**kwargs, so a type checker can reject field typos and value-type
mismatches at the call site. Coexists with filter()/exclude().

Model field declarations across the monorepo drop their primitive
annotation (`name: str = types.TextField()` → `name = types.TextField()`).
The descriptor protocol handles both class- and instance-access typing.
Override equals/not_equal/gt/gte/lt/lte on EncryptedFieldMixin with a
`Never`-typed parameter so a type checker rejects the call at the use
site, and raise TypeError at runtime as a safety net. is_null remains
the only meaningful comparison since ciphertext is non-deterministic.

This makes the design's "method surface = capability" promise actually
load-bearing: misuse is a type error, not a runtime no-op or a silently
empty filter.
Address review findings on the previous commit:

1. The typed `.equals()` block doesn't help users on the legacy kwarg
   path. `filter(api_key='x')` still resolved via the allowed `exact`
   lookup and silently returned zero rows (ciphertext is non-
   deterministic). Wrap the exact lookup class in `get_lookup()` so
   non-None right-hand values raise TypeError at lookup construction.
   None still passes through, preserving the exact-None → isnull rewrite.

2. Strengthen `is_null` test to inspect the Q's children instead of just
   asserting isinstance — a regression in `Field.is_null` would have
   slipped past the old check.

3. Add `assert self.name is not None` in the error-message helper, so a
   call on an unbound field fails loudly instead of rendering "None" in
   the error.

4. Change override return type from `Q` to `Never` to match the
   actually-unreachable return; `Never` is assignable to `Q` so call
   sites like `where(field.equals(...))` still type-check at the use
   site, with the `Never` parameter error as the surfaced diagnostic.

5. Parametrize the ordering-comparison test so each method (gt/gte/lt/
   lte) reports independently.
Extend the typed read API across forward foreign keys:

    Child.parent.name.equals("foo")     →  Q(parent__name="foo")
    Order.user.profile.city.equals(...)  →  Q(user__profile__city=...)

Runtime: new RelatedFieldRef / PrefixedFieldRef helpers proxy class-level
attribute access through to the related model's fields, accumulating a
lookup-path prefix as it goes. ForwardForeignKeyDescriptor gets a
__getattr__ that yields the initial RelatedFieldRef so chaining starts
from `Child.parent`. The SQL builder's existing names_to_path / join
machinery handles the rest — we just produce correctly-prefixed Q.

Typing: ForeignKeyField stub now returns a _ForeignKeyDescriptor[T, V]
with overloaded __get__ — class access yields `type[T]` (so the related
model's typed field surface is visible) and instance access yields V
(T or T | None). __set__ is overloaded to accept V | int so bare PK
assignment still type-checks.

Migration: drops `: ModelType = types.ForeignKeyField(...)` annotations
across the monorepo. The new stub provides the descriptor type and the
overloads handle both class- and instance-side typing.

Scope: forward FK only. Reverse FK (ReverseForeignKey) and M2M
traversal will follow the same pattern in a separate commit.
1. Encrypted-field bypass: PrefixedFieldRef now calls _reject_if_blocked
   in equals/not_equal/gt/gte/lt/lte and the string lookups. If the
   wrapped field is an EncryptedFieldMixin instance, raise TypeError with
   the same "use .is_null() instead" hint the direct-access path gives.
   The SQL-layer block (_exact_for_encrypted) still catches it as a
   second line of defense, but the typed-API now fails at the call site
   for clearer stack traces.

2. Multi-hop coverage: added tests exercising the
   RelatedFieldRef → RelatedFieldRef → PrefixedFieldRef recursion via
   Grandchild.mid_parent.grandparent.name, both as Q construction and
   end-to-end query.

3. Bool slip-through: documented the Python `bool <: int` quirk in the
   _ForeignKeyDescriptor stub comment so future maintainers know the
   runtime check in ForwardForeignKeyDescriptor.__set__ is the only
   guard. (No clean way to exclude bool from `int` in Python's type
   system.)

4. Descriptor attribute shadowing: documented in the RelatedFieldRef
   docstring with a pinned test asserting the current behavior (where a
   field named `field` on the related model would be shadowed by the
   descriptor's own `.field`). The architectural fix — returning a fresh
   proxy from __get__(instance=None) — is bigger and deferred.
Field.__set__ was already a data descriptor at runtime, but its
parameter was typed `value: Any` — so ty silently allowed wrong-type
assignment like `row.name = 123` on a TextField. Narrow to `value: T`
so the type checker enforces the declared field type at the call site.

Plain's runtime is more permissive (`to_python` converts strings → ints
etc.), but encouraging explicit conversion at the boundary catches a
real bug class and makes the new typed-field declarations actually pull
their weight.

Tests added: a TYPE_CHECKING-only block with deliberately wrong
assignments and `# ty: ignore[invalid-assignment]` markers. If the
type-check ever loosens, ty flags the markers as unused suppressions —
making regressions visible.
Master's "Type fields as parameterized descriptors" overlaps heavily with
typed-where's foundation. Resolution keeps typed-where's typed-query
surface (.equals/.contains/...; where(); FK traversal via
_ForeignKeyDescriptor.__get__ -> type[T]) and adopts master's annotation
conventions where they're additive:

- JSONField / EncryptedJSONField: LHS-annotate explicitly (stubs return
  Any).
- String-arg ForeignKeyField: take master's split overloads — bare T
  with required LHS annotation — and apply across affected models.
- TextField.contains/icontains/startswith/endswith: kept (typed-where
  needs them; master had removed them).

Reordered tests/app/examples/models/relationships.py so WidgetTag.widget
uses a class-arg FK (Widget defined first, M2M through="WidgetTag")
preserving type-level FK traversal in test_typed_where_fk after master's
LHS-annotation convention.
Class-level access to a forward FK (Child.parent) now returns a fresh
RelatedFieldRef traversal proxy from __get__ instead of the descriptor
itself. Attribute lookup on the proxy reads the related model with
inspect.getattr_static, so a related field whose name collides with a
public descriptor attribute (field, is_cached, get_queryset,
get_prefetch_queryset) resolves to the field rather than silently
returning the descriptor attribute and building wrong SQL.

Prefetch machinery, the only code that needs the descriptor via
class-level access, now reaches it with inspect.getattr_static in
get_prefetcher to bypass the proxy.
is_in builds a Q(field__in=[...]) membership condition on every field,
typed as an iterable of the field's value type so a wrong element type is
rejected at the call site. Negation composes with ~. FK traversal builds
the prefixed path, and encrypted fields block it with a Never-typed
parameter plus a runtime TypeError, matching the other comparisons.
Add a Querying subsection covering where(), the condition methods on every
field and on text fields, negation and combination, foreign-key traversal,
and the encrypted-field restriction to is_null().
…lution

PrefixedFieldRef now delegates each condition to the wrapped field's own
method and rewrites the resulting Q's leaf keys onto the relation path,
instead of hand-mirroring every condition method. This makes the traversed
surface exactly the field's own surface: a method the field doesn't define
(contains on a non-text field) raises AttributeError through traversal, and
encrypted-field blocking is enforced automatically by the field's own method
bodies — no separate reject hook.

RelatedFieldRef resolves names through the related model's metadata
(get_forward_field) rather than attribute lookup, which is shadowing-immune by
construction. Both refs now require a resolved model class; the dead
string-model branch is gone.

Field._build_q builds its Q via the positional-tuple constructor rather than
poking children directly.
Introduce a minimal generic Selectable[T] marker that Field[T] and
BaseExpression subclass, giving select() a single base to bind each
column's value type against. Because the generic base shifts the C-level
solid base, the Empty stand-ins used by Field and Query cloning subclass
Selectable so their __class__-reassignment stays layout-compatible.
select(*items, flat=False, result_type=None) selects specific columns as
tuples, flat scalars, or dataclass instances via RowQuerySet[R], never
partial model instances. Reuses the values_list plumbing for SQL and row
building; writes and values()/values_list() are blocked on a selected
queryset.

The overload ladder binds each column's type through Field[T]: the pinned
ty build unwraps a Field parameter but not the wider Selectable base
inside an overloaded method on a generic class, so fields get precise
per-column types and expression-containing selects type as tuple[Any, ...].
Runtime tests cover tuple/flat/dataclass modes, expression columns,
chaining with where(), and the error paths. Static-typing tests assert
the precise row types and carry load-bearing ty:ignore markers on the
misuse cases. README documents the three modes and why select() returns
rows rather than partial model instances.
@davegaeddert davegaeddert changed the title Add typed select() for column selection [merge 7] Add typed select() for column selection Jul 23, 2026
@davegaeddert davegaeddert changed the title [merge 7] Add typed select() for column selection [2b] Add typed select() for column selection Jul 23, 2026
Give Selectable[T] a TYPE_CHECKING-only annotated member referencing T so
ty can solve the per-column typevar, then re-key the whole select() overload
ladder from Field[T] back to Selectable[T]. Mixing an expression into a
select() now keeps the field columns precise: select(D.priority, Upper("name"))
types as RowQuerySet[tuple[int, Any]] instead of RowQuerySet[tuple[Any, ...]].

Also:
- Drop the unreachable RowQuerySet guards in values()/values_list() (the
  RowQuerySet overrides are the single enforcement point).
- Build select(result_type=...) dataclasses positionally when there are no
  kw-only fields, deciding the strategy once instead of per row.
- Hoist the related_typed import out of _selectable_to_column so it runs at
  most once per select() call.
- Collapse the duplicated Empty solid-base comments to point at selectable.py.
…pted text surface

Master's ruff 0.16 defaults and ty 0.0.80 landed after this branch:
convert the shadowing migration to tuples, bind the AttributeError probe,
and suppress the Liskov diagnostics the Never-typed blocks exist to cause.

Master reparented EncryptedTextField onto TextField, so it now inherits
contains/icontains/startswith/endswith - block those the same way, and
keep the non-None exact rejection inside master's get_lookups() registry.

Master's migrations-reset tests pinned the examples leaf by name; derive
the leaf, the next number, and the leaf's models from the real history so
adding a migration to examples doesn't break them.
The pending-changes assertion read "Create model " with no name, which
passes no matter what the refusal lists. Read the leaf migration's
CreateModel operations instead, so deleting it has a named consequence
and no branch has to re-pin the model by hand.
The ty: ignore[invalid-method-override] on EncryptedTextField and
EncryptedJSONField is class-wide, so it would also hide a block that
stopped blocking. Each blocked method now has a direct call site carrying
ty: ignore[invalid-argument-type] - ty reports an unused suppression as an
error, so widening any parameter away from Never fails type-check - and the
same calls assert the runtime TypeError, pinning both sides together.
is_null() carries no marker, so a Never creeping onto it breaks the build.

Note on each class-level ignore that it exists for the deliberate Never
narrowing and that any other override on the class needs a hand check.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a4f0fd86d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plain-postgres/plain/postgres/query.py
Comment thread plain-postgres/plain/postgres/query.py Outdated
…lds()

dataclasses.fields() disagrees with the constructor in both directions,
so a dataclass carrying an InitVar was rejected as too narrow:

    @DataClass
    class Stat:
        name: str
        priority: InitVar[int]

    select(D.name, D.priority, result_type=Stat)
    TypeError: ... has 1 fields but 2 columns were selected.

fields() omits an InitVar entirely (it is a constructor parameter that
is never stored) and lists init=False fields (which cannot be passed).
inspect.signature() is what result_type(*row) actually has to satisfy,
so the arity check, the positional name check and the keyword-only
branch all read off it now.

A variadic constructor has no fixed arity to map columns onto and is
refused with its own message rather than failing later.

Reported by Codex on #89.
An annotation appends a column, so the rows gain a member the declared
row type doesn't have. All three modes were wrong, each differently:

    select(D.name).annotate(x=Value(1))
    # declared RowQuerySet[tuple[str]], yields ('alpha', 1)

    select(D.name, D.priority, result_type=NS).annotate(x=Value(1))
    # TypeError: NS.__init__() takes 3 positional arguments but 4 were given

    select(D.name, flat=True).annotate(x=Value(1))
    # annotation silently dropped

annotate() now refuses on a RowQuerySet and names the supported order —
annotate first, then select() — which already worked and is tested.

It joins the other refusals that would change the SELECT list after
select(): values(), values_list(), only(), defer() and select_related()
already raised, so annotate() was the only gap. select() twice stays
last-wins, because there the declared type follows the new columns.

Reported by Codex on #89.
@davegaeddert

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bb3f36523

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plain-postgres/plain/postgres/query.py Outdated
Comment thread plain-postgres/plain/postgres/query.py
…r takes

A constructor mixing positional-only and keyword-only parameters
validated and then failed, because every value was passed by keyword:

    def __init__(self, name: str, /, *, priority: int) -> None: ...

    TypeError: __init__() got some positional-only arguments passed as
    keyword arguments: 'name'

The branch was all-or-nothing — keyword for everything if any parameter
was keyword-only, positional otherwise — and neither call works for a
mixed signature.

Parameter kind decides how each value has to be passed, and a signature
always orders positionals before keyword-onlys, so the row splits at a
single point computed once per query: positional-only and
positional-or-keyword by position, keyword-only by name. The
all-positional case keeps its fast path.

Reported by Codex on #89.
… keep their type

RowQuerySet[R] specializes its base as QuerySet[Any], so every inherited
method annotated -> QuerySet[T] handed back QuerySet[Any] and dropped R.
The same annotation erases a custom queryset subclass: after
CustomQuerySetModel.query.filter(...).reverse(), get_custom() stopped
type-checking on code that works.

Fixed at the base rather than with per-method overrides on RowQuerySet.
Everything that clones through self._chain() returns the same class at
runtime, so it is annotated Self now: reverse(), none(), defer(),
only(), select_for_update(), __and__, __getitem__ (slice), __deepcopy__
and _next_is_sticky. order_by(), distinct(), filter(), all() and the
rest were already Self.

Left alone deliberately:

- values(), values_list() and _values() genuinely change the element
  type, so Self would be wrong.
- __or__ is the one chaining method that cannot be Self. A sliced left
  operand can't be filtered further, so it is re-expressed as an id
  subquery against Meta.base_queryset — a plain QuerySet by design,
  since it must never be a user-defined queryset that might filter rows
  out. That branch really does hand back a different class (verified:
  qs[0:1] | qs returns QuerySet, not CustomQuerySet), so the annotation
  keeps saying QuerySet[T] rather than lying. There is a comment at the
  branch and a corpus claim recording the degradation.

Reported by Codex on #89.
@davegaeddert

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

select() twice is last-wins, but not when the second one selected an
expression:

    D.query.select(D.name).select(F("priority"))
    TypeError: Cannot call annotate() after select()

_values_list aliases an expression column by annotating internally, and
that internal call went through the public annotate() -- which
RowQuerySet refuses. The guard is aimed at a caller appending a column
to a finished row, not at select() rebuilding one.

The mechanism moves to _annotate() and the public annotate() delegates
to it, so _values() can alias expressions without tripping a guard
meant for callers. The guard itself is unchanged and still tested.

Reported by a local Codex review of #89.
A prefetch attaches related objects to each result's attributes, and a
row -- tuple, scalar or dataclass -- has nowhere to put them. The
lookups were retained through select() and applied to the rows anyway:

    Widget.query.prefetch_related("tags").select(Widget.name,
                                                 result_type=NameRow)
    AttributeError: Cannot find 'tags' on NameRow object

Tuple and flat modes didn't raise, which is worse: the prefetch query
ran and its results went nowhere, so it read as working while being
silently wasted work.

Refused in both orders now, the same way select_related() already is,
with a message pointing at selecting the related model's columns
instead. README and the typing corpus record it alongside the other
row-mode refusals.

Reported by a local Codex review of #89.
A selected expression gets an internal alias, and the counter that
generates it only avoided the string columns being selected right now.
It ignored everything already on the query, which broke two ways:

    D.query.select(Upper("name")).select(Upper("status"))
    ValueError: The annotation 'upper1' conflicts with a field on the model

The counter restarts at 1 on every select(), so the second expression
regenerated the first one's alias. The error named neither the real
cause nor a real field. Same for F -> F, flat -> flat, expr ->
result_type, and any third select().

    D.query.annotate(upper1=Lower("name")).order_by("upper1")
           .select(D.name, Upper("status"))

Here 'upper1' is exactly what the generator produces for Upper(...), so
it overwrote the caller's own annotation and the rows came back ordered
by UPPER(status) instead of LOWER(name) -- silently, with no error at
all. The filter variant is the same shape.

The alias now has to clear the query's existing annotations and the
previously selected columns as well as the incoming ones, and each
generated alias is reserved as it is handed out. select() twice is
genuinely last-wins across the whole matrix: field/expr in either
order, same or different expression function, F, mixed lists, flat,
result_type, and three in a row.

_annotate's message was also false on a row-mode queryset, where it
compares against selected columns rather than model fields. It says
which now.

Reported by an Opus review of #89.
_merge_sanity_check only looked at the left operand's row mode, so it
caught one direction of the merge and not the other:

    row_qs | model_qs   -> TypeError, as intended
    model_qs | row_qs   -> RecursionError: maximum recursion depth exceeded
    model_qs & row_qs   -> RecursionError

Either side being in row mode is enough to matter -- the merge produces
a query neither side describes. The check is symmetric now, compares
the selected columns as well as the SQL-level selects, and names both
classes.

Pre-existing: values()/values_list() could already reach it. select()
is just the first typed door to it, so it is fixed here.

Reported by an Opus review of #89.
get_or_none() was the one read method not redeclared, so it came back
as Any | None instead of R | None.

create() and bulk_create() were the two writes not refused, while
get_or_create(), update_or_create(), update(), delete() and
bulk_update() all were. They insert model rows from a queryset that no
longer describes model rows, so they get the same refusal.

Reported by an Opus review of #89.
…duplicates

Tests for every item in the review: the full select()-twice matrix, the
user-annotation cases in both order_by and filter form, merging row and
model querysets in both directions, and the create()/bulk_create()
refusals. Corpus claim for get_or_none().

README: distinct() with an order_by() on an unselected column returns
duplicates, because the ordering column has to join the SELECT list for
Postgres to sort by it and SELECT DISTINCT then dedupes on it too.
Same as values_list(), not new to select().
Takes #133's cross-model guard for where() and #87's lock-method rename.

Conflicts, both kept: query.py's imports needed condition_origins_of
alongside Selectable, and my pre-#87 select_for_update() -> Self became
master's four lock methods, which get the same Self treatment the rest
of the chaining surface got -- rows.for_update() has to keep its row
type like rows.reverse() does.
select()'s half of the guard #133 gave where(). Field[T] carries no
model identity, so this type-checked and then resolved "name" against
DefaultsExample:

    DefaultsExample.query.select(Widget.name)   -> [('alpha',), ...]

Silently the wrong column when both models have one by that name, a
FieldError from the compiler when they don't.

The carrier is already there: Field.source_model, which #133 added for
where() and which reports the declaring model, or for a traversed copy
the root the traversal started from. select() compares it to self.model
and names both, in where()'s wording.

The cross-model check runs before the traversal refusal, so a traversed
column rooted on another model is reported as the cross-model mistake
it is -- "select columns on the queried model" would be advice that
doesn't help. A traversed column on its own root still reports
traversal.

Expressions are left alone: F("email") and Upper("email") take a string
resolved against whatever query they land in, like filter()'s kwargs,
so there is no origin to check.
Five small things a live-database review turned up, none of which
change the shape of the feature.

A field read off a mixin class raised a bare AssertionError with no
message: the mixin holds the declaration and only the model that mixes
it in has an attached, named copy. It is a TypeError that says so now.
(A checker rejects the access too -- __get__ wants owner: type[Model]
-- so this is the backstop for an untyped call site.)

A sliced RowQuerySet in | raised "Cannot call values() after select()".
The id-subquery fallback called the public values(), which select()
refuses; it uses _values() now, the mechanism, like the rest of the
internal paths.

_merge_sanity_check compared columns but not row shape, so merging a
result_type= queryset with a tuple one succeeded and quietly handed
back whichever shape the left operand carried. It compares
_iterable_class and _select_result_type too, with its own message,
since "the same values" isn't what differs.

The expression alias generator now also steps over the model's own
column names, so a model with a column literally called upper1 or f1
can't be shadowed on a first select. AliasCollisionExample exists to
prove it.

The README's lead select() example declared `email: str` rather than
`Field[str]` and carried a `query:` line that isn't needed -- none of
it type-checked. Rewritten, and the same example now lives in the
typing corpus so it can't rot again.
Takes #85's returning() for update()/delete().

Conflicts, all in query.py:

__and__/__or__ were rewritten by #85 with ReturningQuerySet overloads,
so master's versions win wholesale; the only thing carried over is that
their id-subquery fallbacks go through _values() rather than the public
values(), which select() refuses on a row-mode queryset.

That rewrite also settles something this branch had documented as
unfixable: __or__ now returns Self, via an overload pair and a cast on
the base_queryset branch. The corpus claim that recorded the
degradation is now a claim that the row type survives.

update() keeps this branch's message, which names select() alongside
values()/values_list(), plus master's new lock-target rejection.
_clone() carries both branches' state. select() and its helpers sit
beside master's ReturningQuerySet block.

The new AliasCollisionExample migration moves to 0022 and depends on
master's 0021_returningevent -- a package can only have one leaf.
Takes #86's bulk_upsert() and #88's upsert(). Both conflicts in
query.py were import lists; everything else auto-merged.

The structural diff against master turned up what the last two merges
also had -- a silent leftover:

RowQuerySet.update_or_create() was overriding a method #88 deleted, so
it was refusing an API that no longer exists. Removed, with its test.

And the mirror of it: #86 and #88 added bulk_upsert() and upsert(),
neither of which RowQuerySet refused, so both ran on a row-mode
queryset and handed back model instances -- exactly what the
create()/bulk_create()/get_or_create() refusals are there to stop. They
refuse now, with tests.

AliasCollisionExample's migration moves to 0028 on top of master's
0027_upsertstamped, so the examples package keeps a single leaf.
…eryset

bulk_update() already failed, but from the update() inside its own
transaction.atomic(savepoint=False) -- so the refusal left the
enclosing transaction unusable and the *next* query raised
TransactionManagementError. Refusing up front keeps it a plain
TypeError with nothing sent.

returning() was accepted and inert: it captures the rows a write
touched, and select() has already refused every write. Worse
statically, since the checker went on believing update() hands back
instances. Both orders are refused now -- returning() on a RowQuerySet
is typed Never, and select() after returning() raises, which is
runtime-only because ReturningQuerySet is a QuerySet subclass and
select() resolves on it like any other method. The corpus records that
asymmetry rather than implying both halves are typed.
The ladder is ten hand-written overloads with no codegen, and the
corpus only exercised its ends -- one, two, three columns and the cliff
past ten. The middle was unasserted, and provably so: transposing two
typevars in the six-column rung type-checked clean, so a wrong row type
could ship silently. The PR body claimed otherwise.

select_ladder.py asserts all ten against a model with ten distinctly
typed columns, which is what makes a transposition visible -- swapping
two typevars swaps two types in the asserted tuple. Verified by
transposing the six- and eight-column rungs and watching
type-assertion-failure fire for each.
@davegaeddert
davegaeddert marked this pull request as ready for review September 20, 2026 04:22
@davegaeddert
davegaeddert enabled auto-merge (squash) September 20, 2026 04:22
@pullapprove5

pullapprove5 Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor
PASS: 1 review scope passed
Scope Progress
code 1/1

View in PullApprove

Next steps:

@davegaeddert
davegaeddert merged commit 60f1f6c into master Sep 20, 2026
10 of 13 checks passed
@davegaeddert
davegaeddert deleted the select branch September 20, 2026 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant