docs(gfql): comment diet installment 2 — _residual_polars_expr 66 → 27 lines - #1864
Merged
Conversation
… 27 lines Kept, because neither a name nor a test can express it: - expr is a string BY CONTRACT, and why (the #1729 lowering serializes text). - PARITY BY CONSTRUCTION: same parser, same lower_expr, bijection over the same frame, so the row lowering's guards are INHERITED and must NOT be duplicated here. Without this a maintainer reasonably re-implements them. - columns_nan_free opt-out: why THIS lane may skip the NaN mask (ingest already ran _pl_nan_to_null and no projection can introduce NaN), and why a computed operand and the general lowering both keep it. - The NOT-reachable set, so nobody adds dead code for STARTS WITH and friends. Dropped, because tests already assert it: the exhaustive covered-vocabulary list, the itemized decline enumeration, the escaped-literal note -- pointed at test_lowering.py instead. Also dropped the q7 timing claim; receipts live in pyg-bench. bin/ci_docs_only_check.py: AST identical, no code changed. Suite: 0 non-cudf failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
Reviewer was right that tests handle most of it. test_residual_polars_native.py has 69 tests whose NAMES are the spec -- fast_lane_matches_chain_fallback and value_semantics_match_fallback ARE the parity argument; no_such_residual_ever_reaches_the_translator IS the not-reachable paragraph; escaped_literal / dtype_mismatch / declines_and_fallback_raises_designed_error are the decline enumeration. All of that is now a pointer. 66 -> 27 -> 15 lines. What survives is only what a passing test cannot convey: that guards are INHERITED so duplicating one here is invisible to tests, and the columns_nan_free safety argument, whose failure mode is silently wrong answers. Method correction: I should have read the test file BEFORE deciding what was underivable. 'Can a test express this?' is not answerable from the docstring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
This was referenced Aug 10, 2026
pull Bot
pushed a commit
to admariner/pygraphistry
that referenced
this pull request
Aug 11, 2026
… two docstrings Applying the settled rule to row_pipeline.py, largest remaining prose file. Coverage check first (the lesson from graphistry#1864): _order_keys_hold_list_like_values has 18 tests, so its trigger enumeration is already specified -- it becomes a pointer. _single_alias_cache_key had ONE, about clearing rather than keying, so its two real claims were unpinned. A wrong cache key serves a stale expression: a silent wrong answer no existing test would catch. Added test_single_alias_cache_key.py: dtype must be in the key (names alone would serve a float expression for a string column), parameterized dtypes must not collide -- with asserted so the hazard is visible rather than described -- plus expr/alias/nan_free/schema-order, hashability. Both docstrings then drop to ~11 lines, keeping only what tests cannot say: the str(dtype) rationale, why parser availability is deliberately unkeyed, and the directive not to tighten an intentionally-broad decline (over-declining costs a fallback, under-declining returns a wrong ORDER). Registered in POLARS_TEST_FILES and guarded with importorskip -- both halves of yesterday's lane lesson. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
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
1 participant
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.
Second installment. Same standard: a comment earns its place only by stating something a name or a test cannot.
bin/ci_docs_only_check.pyproves no code changed — AST identical to base.Kept (underivable)
expris a string by contract, and why — the feat(gfql): plan connected join predicate pushdown #1729 lowering serializes residuals to text.lower_expras the fallback, over a bijection of the same frame, so the row lowering's guards are inherited and must not be duplicated here. Without that sentence a maintainer reasonably re-implements them.columns_nan_freeopt-out: why this lane may skip the NaN mask (ingest already ran_pl_nan_to_null; projection cannot introduce NaN), and why a computed operand and the general lowering both keep it.STARTS WITHand friends.Dropped (tests already assert it)
The exhaustive covered-vocabulary list, the itemized decline enumeration, the escaped-literal note — pinned in
test_lowering.py, which the docstring now points at rather than restating. Also the q7 timing claim: a receipt, and receipts belong in pyg-bench.Next:
lazy/engine/polars/row_pipeline.py(668 comment+doc lines) andexpr_const_fold.py's design-doc headers, where the (P)/(A)/(E) criterion and the #1802 engine-divergence constraint stay.🤖 Generated with Claude Code
https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi