Honor the saving flag in sum() and random()'s count side effect - #118
Merged
Conversation
count(), count_distinct(), max() and min() all guard their cache writes with `record and self.saving`, so a default table (saving = False) computes statistics on demand without writing to the <table>_counts/<table>_stats cache tables. Two paths broke that contract and wrote regardless: - PostgresStatsTable.sum() recorded on `if record:` alone, missing the `and self.saving` that max()/min() use. - random(), on the uncached non-empty-query branch, materializes the match set to sample from it and calls stats._record_count() with the count it learns as a side effect -- unconditionally, unlike count(). Add the `self.saving` guard to both, so nothing writes to the cache tables while saving is False. With saving = True the behavior is unchanged (both still record). Regressions cover both directions for each method; the two "does not cache without saving" tests fail on master. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
roed-math
pushed a commit
to roed-math/psycodict
that referenced
this pull request
Jul 22, 2026
sum() and random()'s uncached-count side effect now honor the saving flag, matching count()/count_distinct()/max()/min(), so drop the "known wrinkle" note and fold sum + random into the single saving = False rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sum() still promised "compute it, store it" on a cache miss, which the previous commit made conditional; give it a real docstring describing the saving-gated store. The record parameter descriptions on count, count_distinct, max and min (both the stats table and the search-table wrappers) made the same unconditional promise, so state the saving condition uniformly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
roed-math
pushed a commit
to roed-math/psycodict
that referenced
this pull request
Jul 22, 2026
…note The uncached-count bullet still said random() records the computed count unconditionally, contradicting the side-effect note (and, after roed314#118, the code): the write happens only when count-saving is enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
roed-math
pushed a commit
to roed-math/psycodict
that referenced
this pull request
Jul 22, 2026
Rebased onto main, which absorbed everything this PR used to anticipate; the reconciliation: - Version: 1.0.0 is now set in psycodict.__init__.__version__, the single source of truth roed314#121 established -- pyproject keeps main's dynamic version, SPDX license string and >=3.9 floor, and this PR's classifiers drop the License:: entry (setuptools >= 77 rejects it next to an SPDX string) and the 3.8 entry. - README: the rewrite now carries the Versioning.md pointer (roed314#123) in its Documentation list, which switches to relative links -- on the Sphinx site (roed314#122) those resolve to the copied guide pages, and they still render on GitHub; CHANGELOG and LICENSE stay absolute since neither is on the site. Supported Python is 3.9+; blob/master URLs become blob/main. - CHANGELOG: entries for the late wave -- sum/random honoring saving (roed314#118) and the 3.9 floor (roed314#121) under breaking changes; the docs site (roed314#122), Versioning.md/CONTRIBUTING/SECURITY and __version__ (roed314#121) under added; reload metafile+resort (roed314#114), update_from_file stats publication (roed314#115), copy_dumps delimiter escaping (roed314#116) under fixed; config.ini untracking (roed314#120), this release workflow and CITATION.cff (roed314#124) under infrastructure. - release.yml: the flow notes now say bump __init__.__version__ and merge to main. - Fixes the docs build on main, broken by the roed314#119 x roed314#122 crossing (roed314#119 merged after roed314#122's last CI run): the Configuration docstring's nested defaults list needed blank lines to be valid RST under autodoc, and the README's relative LICENSE link had no target on the site. Suite 857 passed / 36 skipped; ruff clean; sphinx -W clean; python -m build + twine check --strict pass with Version 1.0.0, License-Expression GPL-2.0-or-later, Requires-Python >=3.9 and no License classifier in the wheel metadata. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
count(),count_distinct(),max()andmin()all guard their cache writes withrecord and self.saving, so a default psycodict table (saving = False) computes statistics on demand without writing to the<table>_counts/<table>_statscache tables. Two paths broke that contract and wrote regardless ofsaving— the asymmetry flagged in the Searching.md side-notes of #105.The two offenders
PostgresStatsTable.sum()recorded onif record:alone, missing theand self.savingthatmax()/min()use.random(), on the uncached non-empty-querybranch, materializes the match set to draw a uniform sample and callsstats._record_count()with the count it learns as a side effect — unconditionally, unlikecount().Fix
Add the
self.savingguard to both. With the defaultsaving = Falsenothing is written to the cache tables; withsaving = Truebehavior is unchanged (both still record). This makes the whole read API consistent:record/side-effect caching is inert unless a data-management subclass opts in viasaving.Tests
tests/test_stats.pygains four regressions — for each method, one asserting no cache write whensaving = Falseand one asserting the write still happens whensaving = True. The two "does not cache without saving" tests fail on master and pass here.Full suite 742 passed / 27 skipped locally (PG18, psycopg 3.3.4); ruff clean. Based on current
master.The read-API spec in #105 (
Searching.md) is being updated in tandem to drop the "known wrinkle" note and document the now-symmetric behavior.🤖 Generated with Claude Code