test: port analyze_reltuples, projection_update and projection_drop_column - #1133
Conversation
…olumn (commandprompt#432) Three suites, 21 names, each graded `missing: 0` by compare_to_bash.py. One pull request rather than three because the per-PR cost -- the census, the two counts, TESTS.md and its anchor -- is paid once per review and not once per suite. EACH PORT ASSERTS SOMETHING ITS BASH ORIGINAL DOES NOT. analyze_reltuples: the helper builds a columnar and a heap table from two separate inserts and compares their row estimates without ever checking they hold the same rows, so a short insert changes the subject silently. Both counts are asserted now. And the arm named `20 stripes: within 5% of actual` never reads the geometry -- a single-group table passes that 5% check too -- so the port reads pgcolumnar.row_group and fails first if the fixture is not the shape the name claims. The stripe limit does take effect today: measured, 20 row groups with it against 1 without. projection_update: an UPDATE whose WHERE matched nothing leaves both sides identical and every arm below it green, having exercised no fan-out at all -- the suite would report success for the defect it exists to catch. The affected row counts are pinned. projection_drop_column: the two non-owner arms exist to show a stranger cannot tell a projected column from an unprojected one. The bash suite asserts each against the literal 42501 and leaves the reader to notice they are the same string; the port compares them to each other, so indistinguishability is asserted rather than implied. It also reads exc.sqlstate instead of parsing `VERBOSITY verbose` output with sed. AND THAT PORT'S OWN PREMISE CAUGHT ME. I first asserted the non-owner could read the table; the run returned 42501. USAGE on the schema resolves the NAME, while SELECT is a separate grant the role does not have. What the arms below actually need is only that the name is not invisible -- 42P01 would mean they were asserting ownership against a table the role cannot see -- so the premise tests for that instead. SET ROLE rather than a login role, which is the opposite of test_projection_privilege.py, and the two precedents disagree so the choice is stated in the module: real logins are needed when the ACL layers are the subject, and only add ways to fail when ownership is. Row sets are compared as sorted tuples in Python rather than through pgc_set_hash, so the two harnesses stay independent by construction and a failure prints the rows that differ rather than two unequal hashes. Verified: compare_to_bash missing: 0 on all three registration guards 96 passed, 321 checks guard half 380 passed against its stated 380, 1021 checks cluster half 421 passed against its stated 421, 1218 checks docs_style.sh 47 checks, PASSED No bash suite changes, so no ledger row moves and the census does not. cluster_tests 418 -> 421, re-derived by collection rather than by adding three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
|
Reviewed the content; holding the approval only until The three vacuity findings are real. I checked each against the bash source.So all three hold, and each is worse than it first reads:
Porting turned up three of these in three suites, which is the argument for The premise that caught you is the one I would have got wrong too
The two stated choicesBoth right, and I am glad they are written down rather than merely made.
Comparing row sets as sorted tuples rather than through One PR rather than threeAgreed, and the reasoning generalizes: the census, the two collection counts, I will approve and merge once the two suites jobs report. |
jdatcmd
left a comment
There was a problem hiding this comment.
Both suites jobs are in, 14/14 SUCCESS. Approving on the verification in my earlier comment: the three vacuity findings each check out against the bash source (row_group 0, count(*) 0, affected-row asserts 0), so all three ports assert something their originals did not.
|
Both suites jobs have reported. All 14 green on Nothing changed since your read; the two were queued behind the build matrix. One thing your check adds that mine did not. I found the three vacuity gaps by porting each suite and asking what the port had to assert to be honest. You found them by grepping the bash source for the thing that was absent: That is the cheaper instrument and it generalises: "does this suite read the object its check is named after" is answerable without porting anything, across all 242 remaining suites at once. If the answer rate is anything like three-in-three, it is worth running before the next batch rather than after — it would pick the queue by where the coverage is weakest instead of by name count, which is a better ordering than the one I have been using. I will run that sweep and post the number on #432. |
Three suites ported, 21 names, each graded
missing: 0. Part of #432.One pull request rather than three, because the per-PR cost — the census, the two
collection counts, TESTS.md and its anchor — is paid once per review and not once per
suite. That is the lever I argued for on #432 after measuring that the port finishes by
clearing a tail of 13-name suites rather than the top of the queue.
Each port asserts something its bash original does not
That is the point of porting rather than translating, and it is what makes the result
worth more than the suite it replaces.
analyze_reltuples— the bash helper builds a columnar and a heap table from twoseparate inserts and compares their row estimates without ever checking they hold the
same rows. A short insert on one side changes the subject and every arm stays green.
Both counts are asserted now.
And the arm named
20 stripes: within 5% of actualnever reads the geometry. Asingle-group table passes a 5% check too, so the name was the only thing claiming twenty
stripes. The port reads
pgcolumnar.row_group. The limit does take effect today —measured, 20 row groups with it against 1 without — which is why this is a premise and not
a bug report.
projection_update— an UPDATE whoseWHEREmatched nothing leaves both sidesidentical and every arm below it green, having exercised no fan-out at all. The suite
would report success for exactly the defect it exists to catch. The affected row counts
are pinned.
projection_drop_column— the two non-owner arms exist to show a stranger cannot tella projected column from an unprojected one. The bash suite asserts each against the
literal
42501and leaves the reader to notice they are the same string; the portcompares them to each other, so indistinguishability is asserted rather than implied. It
also reads
exc.sqlstateinstead of runningpsqlwithVERBOSITY verboseandextracting the code with
sed.That port's own premise caught me
I first asserted the non-owner could read the table. The run returned
42501:USAGE on the schema resolves the name; SELECT is a separate grant the role does not
have. What the arms below actually need is only that the name is not invisible —
42P01would mean they were asserting ownership against a table the role cannot see. The premise
tests for that instead, and the comment says why, because the wrong version of it looks
just as reasonable.
Two choices worth stating
SET ROLE, not a login role — the opposite oftest_projection_privilege.py, and thetwo precedents genuinely disagree, so the module says which and why: real logins are
needed when the ACL layers are the subject (that file measured
SET ROLEarms stillpassing with
NOLOGINinjected), and only add ways to fail when ownership is.Row sets compared as sorted tuples in Python, not through
pgc_set_hash. The twoharnesses stay independent by construction rather than by agreement, and a failure prints
the rows that differ instead of two unequal hashes.
Verification
No bash suite changes, so no ledger row moves and the census does not.
cluster_tests418 → 421, re-derived by collection rather than by adding three.Note for whoever lands this and #1127
Both add TESTS.md sections. This one takes 53, 54 and 55; #1127 wants 53. Whichever is
second renumbers, heading and contents anchor. A naive keep-both resolution is caught by
test_docs_cover_the_corpus.py::test_the_contents_list_is_numbered_in_orderrather thanshipped — I checked that by breaking the document on another branch.
🤖 Generated with Claude Code
https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs