Skip to content

Fix timestamp index bounds for non-timestamp sort keys - #437

Merged
tonyalaribe merged 1 commit into
masterfrom
fix/timestamp-guard-current-20261026
Oct 26, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
fix/timestamp-guard-current-20261026

Conversation

@tonyalaribe

@tonyalaribe tonyalaribe commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Indexed latest-page planning used a timestamp-derived bound for other descending sort keys. Sorting by context___is_remote then failed with an unsupported timestamp-to-Boolean cast.

Restrict this optimization to the timestamp column. Extend the existing indexed/raw fixture to cover Boolean and Int32 sorts while retaining timestamp narrowing and deletion-winner assertions. Scope development MinIO stops to the shared server command so isolated fixtures survive local test setup.

Validation:

  • The added fixture fails on deployed base a0da0fc9 with the exact cast error.
  • All 11 targeted merge and ordering tests pass after the guard; formatting passes.
  • The actual Makefile stop recipe passes a nonmutating selector check.
  • All five exact-source local code gates pass: formatting, cargo lint, 2,892 native tests, 16 doctests, PostgreSQL smoke, and 85 end-to-end tests. Native retains 34 skips and one leak diagnostic. The existing deletion-vector fixture passes its configured retry after an initial no-source-files failure; retain that failure.
  • Full make ci-signoff CHECKS="fmt clippy test pg-smoke e2e" passed, including the Linux production image build and real PGWire smoke check. The signed-off image is published.
  • Linux amd64 runtime: ghcr.io/monoscope-tech/timefusion@sha256:13b17070cd972d42fc87d76fcca2075e6df97b85617efb3ea74150731756e83a.
  • Preserve the current production boot for its mature resource observation before another rollout.

This carries the pending correctness work from draft PR #428 onto the deployed aggregate-alias correction. It also consolidates the latest implementation and acceptance evidence in the existing plan. Experimental body-search and browser optimizations remain separate.

@claude

claude Bot commented Oct 26, 2026

Copy link
Copy Markdown

Review

Code: The fix is correct and minimal. narrow_descending_fetch now returns Ok(None) unless the sort column is timestamp, so other sort keys fall back to the unnarrowed plan. That is the safe default, and the guard sits before any table or time-range work.

Test: The new loop in a_newest_first_page_narrows_to_the_index_bound_and_matches_baseline compares indexed and raw results for Boolean and Int32 sorts. The PR says it fails on the base with the exact cast error, which is the red-first evidence the repo asks for. The timestamp narrowing and deletion-winner assertions are kept.

  • The ORDER BY ... DESC NULLS FIRST, id tiebreak makes the comparison deterministic. That is good.
  • If it is cheap, make the guard cover a column named something other than timestamp that is still timestamp-typed. The check is by name, so that column would also skip narrowing. That is the safe side, but it is worth knowing.
  • Compare column.name against the schema's actual time column constant if one exists, rather than a string literal.

Makefile:

  • The pkill pattern ^minio server /tmp/minio-data --console-address :9001$ matches the full command line only if pkill -f sees exactly that string. This holds for the nohup'd command. Any extra flag or an env-wrapped invocation would silently stop matching, and || true would hide it. The PR says a selector check was run, which is reassuring.
  • The pattern is duplicated in minio-start and minio-stop. A shared variable would stop the two from drifting.

Docs:

  • docs/plans/2026-10-19-next-work.md gains about 540 lines of dense operational log. It is unrelated to the fix and makes review hard. Consider splitting it into a docs-only PR, or trimming it.
  • The "retain that failure" note about the deletion-vector retry points to a flaky test. That deserves its own tracking issue rather than a line in the plan.

Status: The PR says the Linux image build and smoke gate are still running and it is in draft. I'd wait for those before merge. I did not run the tests myself.

No security or performance concerns. This is a small, well-guarded correctness fix. The docs bulk and the two small nits above are the only things I'd raise.

@tonyalaribe
tonyalaribe marked this pull request as ready for review October 26, 2026 03:19
@monoscope-bot

monoscope-bot Bot commented Oct 26, 2026

Copy link
Copy Markdown

Monoscope production impact · Coverage unknown

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 5c864b8ea281a00be8e7c0881f69d43c70f7082c · advisory

Evidence window (UTC): 2026-10-19 03:20:02.16654216 UTC → 2026-10-26 03:20:02.16654216 UTC

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@tonyalaribe
tonyalaribe merged commit 5877d77 into master Oct 26, 2026
13 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.

1 participant