Skip to content

test: a rollup's min and max keep each source column's collation across a refresh, in both twins (#1341) - #1372

Merged
jdatcmd merged 41 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-rollup-collation
Oct 15, 2026
Merged

jdatcmd merged 41 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-rollup-collation

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

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_rollup builds 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.

  • Why: d.c is min() 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.
  • Probe on PG17: an en_US database, a "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)

  • The fixture: one "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'||g into the same bucket; under C, B sorts first, under en_US, a does.
  • Arm: the rollup's min/max over both columns equals a recompute after the folding refresh.
  • Premises: the second refresh folded into the existing bucket, and the two collations really disagree on these rows. That one compares both minimums in "C"; a bare comparison of the two columns has no collation to use, and my first draft hit exactly that.
  • No en_US: the shell names a SKIP for each check; pytest declares 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:

main this branch
rollup.sh / test_rollup.py 78/78, 1 passed "got [a1|B999|a1|B999] want [B1|a999|a1|B999]": the C column's min and max came out in en_US order

Evidence

  • Clean: rebased onto ff09c2c, with rollup.sh and test_rollup.py merged 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.
  • Ledger: three rows, one dated: 2353 → 2356 rows, the census 1798 → 1800.
  • Gate: rc=0.
  • Counts: TESTS.md row, and cluster_tests 533 → 534. Several of my PRs take 534, so each restates on rebase.
  • Database-free set: 429 passed.

🤖 Generated with Claude Code

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. "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. With RETURN 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.
  2. Rebase: it conflicts with main 9b3c181. Main is at 1821 never, ceiling 245. cluster_tests 534 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.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Taken, and restated on main: now 3f34821.

  • "the second refresh folds new rows into the existing bucket" is a check now, renamed in both twins, in the shell's no-en_US skip list, and in the ledger with its history. With your mutation (RETURN folded + 1;), both twins are red on it (got [2001|1] want [2000|1]), and it's dated by it. The same mutation reddens fifteen other rollup arms that count what a refresh folded; all of them already had dates. The collation premise stays, since it depends only on core collations and the data.
  • Runs: rollup 85/85 and test_rollup.py 2 passed on each of 15–19. Main hasn't touched either rollup file since this branch's base, so those runs are of the merged files.
  • Restated: cluster_tests 543 → 544; ledger 2516 rows, never 1931; ceiling 242. The gate is clean, and the guard set is 433/433.

On the older rollup premises you flagged (rollup.sh 194, 221, 281, 346): I'll claim them on #1339 and do them after this round.

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 14, 2026
…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
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 808ee16 to 3f34821 Compare October 14, 2026 20:04
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 14, 2026
…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 jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 3f34821 to 2605a26 Compare October 15, 2026 16:27
@OffgridwithJD

OffgridwithJD commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto 57f6e0e (#1401), on top of #1403. The new head is 2605a26. Only the count files changed, so this needs your re-approval.

  • Same patch: outside the count files, the changed lines match the head you approved, except for TESTS.md section renumbers where main's new sections took the old numbers.
  • The ledger is restated on main's census and agrees with its tsv: 2789 rows, 2096 never, 693 dated, read back from the file. Each changed paragraph's arithmetic starts from main's last numbers.
  • cluster_tests 567, by collection over the cluster files on this tree. guard_tests 433 is unchanged.
  • Runs on the rebased tree at 217f23b, PG17 and PG18: PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG17 pytest: 4 passed in 31.27s; PG17 rollup: 107 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 pytest: 4 passed in 30.79s; PG18 rollup: 107 passed + 0 failed + 0 unrunnable + 0 skipped. harness_selftest 1245/1245 and docs_style passed on both. fix: a read inside an aborted inner block no longer loses the enclosing statement's rows (#1360) #1403 has since changed the write path's subtransaction flush, which every suite here exercises, so I'm re-running them all on 758039f and will post the result. CI runs PG17 and PG18 on this head.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Re-run finished on the tree after #1403 (758039f or later). Everything is green, so the head above stands:

  • PG17 docs_style: passed
  • PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG17 pytest: 4 passed in 32.40s
  • PG17 rollup: 107 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 docs_style: passed
  • PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 pytest: 4 passed in 26.91s
  • PG18 rollup: 107 passed + 0 failed + 0 unrunnable + 0 skipped

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 2605a26 to 254c5f3 Compare October 15, 2026 17:06
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved at 254c5f3. Compared with 3f34821, the head I approved, file by file: the changed lines are the same, apart from the count files (check_ledger_budget.txt, expected_tests.txt) and, in #1378/#1379/#1380/#1383, the TESTS.md section number, now 115.

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 254c5f3 to 3563dd1 Compare October 15, 2026 17:53
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved at 3563dd1, the rebase onto 1f7b5da. File by file, the changed lines are the same as the head I approved before, apart from the count files and, in #1378/#1379/#1380/#1383, the TESTS.md section number.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

The PG18 re-run on this head (3563dd1, on 1f7b5da with #1412's index-build change) is all green:

  • docs_style: passed
  • harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • pytest: 4 passed in 15.88s
  • rollup: 107 passed + 0 failed + 0 unrunnable + 0 skipped

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 3563dd1 to 79c1374 Compare October 15, 2026 18:28
jdatcmd and others added 3 commits October 15, 2026 21:18
…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>
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Restated for the chain you asked for. The new head is b3f4d6c, so please re-approve this head.

🤖 Generated with Claude Code

OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
OffgridwithJD pushed a commit to OffgridwithJD/pgcolumnar that referenced this pull request Oct 15, 2026
…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
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from 3173339 to b3f4d6c Compare October 15, 2026 21:23

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jdatcmd and others added 16 commits October 15, 2026 22:00
…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
…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
@OffgridwithJD
OffgridwithJD force-pushed the test/1341-rollup-collation branch from b3f4d6c to f07760f Compare October 15, 2026 22:02
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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 jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jdatcmd
jdatcmd merged commit 0ade382 into commandprompt:main Oct 15, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants