Repository navigation
test: a rollup's min and max keep each source column's collation across a refresh, in both twins (#1341) - #1372
Conversation
jdatcmd
left a comment
There was a problem hiding this comment.
The arm is sound, but one new premise: goes red under a product change (#1236). Reviewed on PG18 at 808ee16.
The arm works. Green: shell 85/85, pytest 2 passed. I forced COLLATE "default" into the refresh fold for text columns (pgcolumnar--1.0-alpha5.sql ~2676). Both twins fail:
FAIL min and max over a C column and an en_US column keep each column's own order across a refresh: got [a1|B999|a1|B999] want [B1|a999|a1|B999]
What to change:
- "premise: the second refresh folds new rows into the existing bucket" compares
refresh_rollup()'s return value and the bucket count. Both are product outputs. WithRETURN folded + 1;(~line 2842), it goes red:
FAIL premise: the second refresh folds new rows into the existing bucket: got [2001|1] want [2000|1]
Drop the prefix in both twins and in the ledger row, and date it with that mutation. The collation premise depends only on core collations and the data, so it can stay. - Rebase: it conflicts with main 9b3c181. Main is at 1821 never, ceiling 245.
cluster_tests534 also collides with #1373.
The older premises in rollup.sh (lines 194, 221, 281, 346) look loose in the same way. They aren't this PR's to fix, but they belong on #1339.
|
Taken, and restated on main: now 3f34821.
On the older rollup premises you flagged ( 🤖 Generated with Claude Code |
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
808ee16 to
3f34821
Compare
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
jdatcmd
left a comment
There was a problem hiding this comment.
Reproduced at 3f34821 on PG18. With RETURN folded + 1;, rollup is 69 + 16 failed, including the second refresh folds new rows into the existing bucket: got [2001|1] want [2000|1]. That's the sixteen you describe. The rename is in both twins and the skip list, and the collation premise rightly stays. Approving. I'll merge once CI is green, after checking the approval is on this head.
🤖 Generated with Claude Code
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
3f34821 to
2605a26
Compare
|
Rebased onto 57f6e0e (#1401), on top of #1403. The new head is 2605a26. Only the count files changed, so this needs your re-approval.
🤖 Generated with Claude Code |
|
Re-run finished on the tree after #1403 (758039f or later). Everything is green, so the head above stands:
🤖 Generated with Claude Code |
2605a26 to
254c5f3
Compare
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
|
Rebased onto 7da848d (#1432, #1409). The new head is 254c5f3; only the count files changed, so please re-approve this head.
🤖 Generated with Claude Code |
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
254c5f3 to
3563dd1
Compare
|
Rebased onto 1f7b5da (#1411, #1412, #1417, #1418). The new head is 3563dd1. Only the count files changed, plus TESTS.md section numbers where main took 115, so please re-approve this head.
🤖 Generated with Claude Code |
|
The PG18 re-run on this head (3563dd1, on 1f7b5da with #1412's index-build change) is all green:
🤖 Generated with Claude Code |
|
Rebased onto 37ea557 (#1414, #1425). The new head is 79c1374. Only the count files changed, so please re-approve this head.
🤖 Generated with Claude Code |
3563dd1 to
79c1374
Compare
…0) and leaves nothing declared, in both twins (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts connections (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Restated for the chain you asked for. The new head is b3f4d6c, so please re-approve this head.
🤖 Generated with Claude Code |
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
3173339 to
b3f4d6c
Compare
jdatcmd
left a comment
There was a problem hiding this comment.
Re-approved at b3f4d6c, its place in the chain on 17420be. This PR's own commits, measured from the link below it, change the same lines as the head I approved before, apart from the count files and the TESTS.md section number.
🤖 Generated with Claude Code
…emise: prefix (commandprompt#1341) Review of commandprompt#1375 by jdatcmd: CI's suites (PG 17) ledger gate found the row seeded as 'premise: the drop really removed the projection' while projections.sh:410 and test_projections.py:795 name the check without the prefix, so the gate saw one check not in the ledger and one orphan row. The row is renamed and keeps its other fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e writing any statistics, in both twins (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…blings do (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… dated (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ys so (commandprompt#1341) test_a_named_column_that_does_not_exist_is_refused calls pgcolumnar.analyze(), which needs PostgreSQL 18, so it declines on 15, 16 and 17 like the other analyze_function twins. It was missing from expected_unrunnable.txt, so CI's PG17 cluster job collected and passed 578 and still exited 67 on the unexpected decline. Measured on PG17: test_analyze_function.py exits 67 without these rows and 0 with them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tored one (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dated (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…its bar, and four that had none get arms here, in both twins (commandprompt#1386) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…eir mutations (commandprompt#1386) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…te, expire or a refresh lose the premise prefix, in both twins (commandprompt#1339) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…ite and expire mutations (commandprompt#1339) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…ss a refresh, in both twins (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lumns' collations differ (commandprompt#1341) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…h twins and the skip list (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
…e more than it folded (commandprompt#1372 review) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VjTxfQJj5pnii9wzmPP9Ho
b3f4d6c to
f07760f
Compare
|
Restacked onto #1375's fix (875429d, its projections row renamed). The new head is f07760f, so please re-approve this head. This PR's own commits are unchanged, and nothing in its diff moved. 🤖 Generated with Claude Code |
jdatcmd
left a comment
There was a problem hiding this comment.
Re-approved at f07760f. This PR's own commits are the same as the head I approved, rebased onto the fixed #1375.
🤖 Generated with Claude Code
Part of #1341: the High time-series item "create_rollup on a source column with a non-default collation".
Measured first: no product bug
create_rollupbuilds each min/max partials column from the type alone, so the partials carry the database's default collation. I expected a refresh's fold,least(p.c, d.c), to compare in the wrong collation. It doesn't.d.cismin()of the source column, so it carries that column's implicit collation. PostgreSQL resolves the default against a non-default implicit collation in favour of the non-default one, so the fold compares in the source's."C"column and an en_US column, with lower-case rows refreshed first and upper-case rows then folded into the same bucket. The rollup equals a full recompute in both.So this is arms only.
The arm (both twins; a new pytest function,
test_min_and_max_keep_each_columns_collation)"C"column and one en_US column, so whichever is the database default, the other differs from it. Rows go'a'||g, refresh, then'B'||ginto the same bucket; under C,Bsorts first, under en_US,adoes."C"; a bare comparison of the two columns has no collation to use, and my first draft hit exactly that.cannot_run,MISSING_DEPENDENCY.Mutation
The fold compares in the partials' default collation:
COLLATE "default"on both arguments, added only for a collatable partials column so numeric min/max is untouched. PG17 (en_US database), main against this branch:Evidence
rollup.shandtest_rollup.pymerged with test: create_rollup's last refusals and rebase_rollup's, in both twins (#1341) #1359's arms. rollup 85/85 and test_rollup.py 2 passed on each of PG 15–19.cluster_tests533 → 534. Several of my PRs take 534, so each restates on rebase.🤖 Generated with Claude Code