Fix identifier composition, table-specific statistics, log state and random edge cases

[roed-math]

First of the rc3 stabilization PRs, covering Track A of the review.

Four independent defects, each verified against the code before changing it and
each covered by a regression test that fails on rc2. Nothing here depends on the
later tracks, so it can land on its own.

A1. max_id / min_id composed their table argument as text

self._execute(SQL("SELECT MAX(id) FROM {}".format(table)))

Both take a public table= argument. A name needing quotes was a syntax error,
and a name carrying its own statement ran it. Both use Identifier now.

While there I documented the empty sentinel, because A4 turned on it: max_id
returns -1 for an empty table, and min_id returns 0 — which is not a
sentinel, since 0 is a real id.

A2. _approx_most_common scaled every table by nf_fields

The frequencies came from the owning table (pg_stats ... tablename = %s) but
the row count came from a literal regclass 'public.nf_fields'. So for any
table other than nf_fields the estimate was that table's frequencies
multiplied by an unrelated row count — silently wrong, and wrong by whatever
ratio the two tables' sizes happen to have.

The count now comes from the table the statistics describe, looked up by name in
the current schema. The column type interpolated into the unnest cast goes
through column_type_sql (from #133) instead of being concatenated raw.

I dropped the planned "quoted table name" test for this path: since #136 a new
search table name must match the lowercase grammar, so such a table cannot be
created, and the row count is bound as a value rather than spliced.

A3. update_from_file mutated a shared default

logging={"operation":"file_update"}   # then: logging["aborted"] = True, logging["logid"] = logid

Consecutive default calls saw the previous call's logid and aborted, and a
caller-supplied dictionary came back modified. Now logging=None with a fresh
mapping per call, copied when supplied, named log_data internally so it is not
confused with the logging module.

Per the handoff's "do not do a large cosmetic rewrite", I ran an AST scan over
the whole package for parameters with mutable defaults that are actually mutated
(item assignment or a mutating method call, discounting rebinding). This was the
only one. Nothing else changed.

A4. Random selection edge cases

1. random(query, pick_first=...) called random.choice([]) when the query
   matched nothing → IndexError instead of the documented None.
2. if maxid < 1: return None reported a table whose single row has id 0 as
   empty. The sentinel is -1, so the test is < 0.
3. if res: discarded a row whose projection was 0, False, "" or []. A
   table full of them exhausted maxtries and raised "Random selection failed!". Now is not None.
4. An unknown mode matched no branch and random_sample returned None,
   which is indistinguishable from an empty result. It now raises ValueError
   naming the accepted modes, before doing any work.
5. Repeatable choice sampling called global random.seed(...), so asking for
   a reproducible sample made every later random number in the host program
   repeat. It uses a local random.Random(repeatable).

Tests

New tests/test_correctness.py, 28 cases. 18 of them fail on rc2 — the
other 10 assert adjacent behavior that was already right and guard it.

Coverage includes: max_id/min_id against table names with spaces, embedded
double quotes, ;, --, /* and non-ASCII characters, plus an injection
attempt asserting the marker table does not exist afterwards; two tables of very
different sizes with the same distribution, checking each estimate brackets its
own row count; log-state isolation across two default calls and a caller
dictionary unchanged after both success and failure; every random edge case
above, including that the global RNG stream is bit-identical across a repeatable
sample.

Full suite: 1297 passed, 36 skipped. Ruff and the -W docs build clean.

🤖 Generated with Claude Code

[read-the-docs-community]

Documentation build overview

📚 psycodict | 🛠️ Build #33921026 | 📁 Comparing 6c3f8d5 against latest (a161f5b)

  🔍 Preview build  

8 files changed · ± 8 modified

± Modified

• genindex.html
• api/searchtable.html
• api/table.html
• api/validation.html
• _modules/psycodict/searchtable.html
• _modules/psycodict/statstable.html
• _modules/psycodict/table.html
• _modules/psycodict/validation.html

[roed-math]

All three review items are addressed in 6c3f8d5.

1. A real row whose scalar projection is SQL NULL

random()'s no-query loop now separates "no row has this id" from "the row is
there and the column is NULL", and only pays for the distinction when the
projection makes it ambiguous:

scalar = projection == 0 or isinstance(projection, str)
for _ in range(maxtries):
    rid = random.randint(minid, maxid)
    res = self.lucky({"id": rid}, projection=projection)
    if res is not None:
        return res
    if scalar and self.exists({"id": rid}):
        # A real row whose projected column is NULL.
        return None
raise RuntimeError("Random selection failed!")

The extra query happens only on the None path and only for a scalar
projection: a projection returning a dictionary already tells the two cases
apart by its shape, since lucky returns None for a missing row and a
dictionary (possibly {} under include_nones=False) for a real one.

Checking on each miss rather than once at the end is deliberate. If the loop
kept retrying past a NULL, it would keep going until it found a row with a
non-NULL value, which biases the sample away from NULL rows; returning
None for the row it landed on is the value that row actually has, and is what
lucky returns for it. The random docstring now says so.

New test test_random_returns_none_for_a_null_scalar_projection: 20 rows with
consecutive ids 0..19 and num NULL in all of them, asserting
table.random({}, "num") is None ten times over. It raises
RuntimeError("Random selection failed!") on a36cb8a.
test_random_retries_a_missing_id_before_giving_up covers the other half:
ids 1..18 deleted, so most draws miss, and random({}, "num") still has to come
back with one of the two surviving values rather than None.

2. The array cast type

New psycodict.validation.array_cast_type_sql(typ), built from the validated
pieces rather than by appending to a declaration:

• text to text[]
• integer[] to integer[][]
• text COLLATE "C" to text[]
• text[] COLLATE "C" to text[][]

validate_column_type already read a declaration in layers (collation off the
end, then array brackets, then a scalar type). That decomposition moved into a
shared _split_column_type, which returns (scalar, depth, collation, cost).
validate_column_type reassembles all of it; array_cast_type_sql drops the
collation and emits scalar + "[]" * (depth + 1). Both go through
_type_sql, so a fixed type still interpolates the preconstructed SQL object
from the closed mapping. No catalog query, no change to the type grammar, and
no string surgery outside the validation module: statstable.py now formats
{1} instead of {1}[] and passes array_cast_type_sql(self.table.col_type[col]).

New test test_approx_most_common_handles_a_collated_column, parametrized over
text COLLATE "C" and text[] COLLATE "C". It adds the column with
add_column on the live table object (no reconnect), inserts 200 rows over
three distinct values, runs ANALYZE, and asserts _approx_most_common returns
all three with estimates summing to the table size. Both parametrizations fail
on a36cb8a with the syntax error from text COLLATE "C"[].

3. The identifier-injection test

The payload now starts with a table that exists and terminates that statement
cleanly:

injected = "%s; CREATE TABLE %s (x int); --" % (empty_table.search_table, marker)

Verified against base a161f5b: the test fails there with DID NOT RAISE Exception, and an instrumented copy of it shows max_id returning -1
(the injected SELECT succeeded) with _table_exists(marker) then True, so
the marker statement really does execute on the vulnerable code. On this branch
Identifier quotes the whole payload as one relation name, max_id raises, and
the marker is never created. Cleanup moved into a finally block, so a run
against vulnerable code leaves nothing behind (confirmed: no marker_*
relations after the base run).

Also in that file, as asked: the module docstring no longer claims every test
fails on rc2, and the comment in the approximate-statistics test now says the
estimates for the two boolean values sum to the table size rather than that
every row has flag set.

Results

Local, against PostgreSQL 18 on a private cluster with
PSYCODICT_TEST_DB_REQUIRED=1:

• pytest tests/test_correctness.py -v: 32 passed (28 before, 4 added)
• pytest tests/ -v --durations=10: 1301 passed, 36 skipped
• ruff check .: clean
• python -m sphinx -W --keep-going -b html docs docs/_build/html: build succeeded

Pre-fix runs of the new tests, for the record: 3 failed / 29 passed at head
a36cb8a (both collated parametrizations and the NULL projection), and the
injection test fails at base a161f5b.

CHANGELOG entries for the two behavior changes are updated in the same commit.

CI on 6c3f8d5 is green across all 20 checks, including the full PostgreSQL
13 through 18 matrix and both downstream suites.

[roed314]

GPT signed off.