Skip to content

Fix indexed sorting and isolate development MinIO stops - #428

Closed
tonyalaribe wants to merge 14 commits into
masterfrom
fix/timestamp-narrowing-20261025
Closed

tonyalaribe wants to merge 14 commits into
masterfrom
fix/timestamp-narrowing-20261025

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

Indexed queries sorted by a nullable Boolean or integer column can fail because newest-first narrowing casts a timestamp bound to the sort column. Restrict this optimization to the timestamp sort key, while preserving timestamp-page narrowing.

Scope development MinIO stop commands to the exact shared-server command so private fixtures survive. Update the existing work plan with delivery evidence and unresolved acceptance items.

Validation:

  • The original Boolean-sort query reproduces the unsupported timestamp-to-Boolean cast.
  • The corrected indexed/raw fixture passes Boolean and Int32 sorts, timestamp narrowing, and deletion-winner checks.
  • Combined-tree make ci-signoff CHECKS="fmt clippy test pg-smoke e2e" passed at 537ef2c1: formatting, lint, 2,888 native tests, 16 doctests, PG smoke, 85 end-to-end tests, and 34 helper tests.
  • The production candidate built, passed its smoke test, and uploaded successfully. Image fingerprint: 794357e9e9fd32f7dd01b220139d2e5eedcca29e0b65a031b6873a650e6009f9.
  • No calibrated browser, thirty-day search performance, matched-resource, or 24-hour recurrence pass is claimed.

@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Review

Overall: The code change is small, targeted and correct as far as I can tell. A few notes below.

src/read/bounded_merge.rs

  • The guard column.name != "timestamp" in narrow_descending_fetch fixes the bug. The lo bound comes from the timestamp filters, so it can't narrow a page ordered by any other column. Returning Ok(None) falls back to the normal plan, which is the safe default.
  • The check is a hard-coded string. If the schema has a named timestamp constant, or the ascending narrowing path has a similar helper, use that so the two paths can't drift apart. Please also confirm that the ascending path (the one the test comment says shares NarrowedFetchExec) doesn't need the same guard.
  • This check compares the sort column's name to the string. Please confirm that latest_page can't return a column that is a different column aliased as timestamp, or a multi-key sort where timestamp is not the leading key. In the second case, narrowing by a timestamp bound would still be wrong. Ordering by timestamp DESC, id looks like the intended shape, but it isn't covered by a test.

Tests (tests/suite/tantivy_e2e_test.rs)

  • The new loop covers a nullable Boolean and an Int32 sort, comparing indexed against raw. That matches the reported failure.
  • Per the repo's bug-fix workflow (step 5), the PR description says the original Boolean query reproduces the cast error. That is good. Please also state that this specific test goes red with the guard removed (the description implies it, but doesn't say).
  • Suggested addition: a case with ORDER BY timestamp DESC, id to pin down that the narrowing still applies when timestamp leads. The existing assertion that fewer than 30 hits are resolved covers the plain case only.

Makefile

  • The anchored pkill -f '^minio server /tmp/minio-data --console-address :9001$$' is more precise and the $$ escaping is right. The pattern is now coupled to the exact start command. If someone changes the flags or data dir in minio-start, the stop pattern will silently stop matching, and || true hides that. Consider a Make variable shared by both targets.
  • If MinIO is launched via an absolute path (/usr/local/bin/minio server …), the ^minio anchor won't match. The nohup minio server form in this Makefile is fine, but a user-started instance could be left running.

docs/plans/2026-10-19-next-work.md

  • This adds about 220 lines of dense progress log, much of it unrelated to this PR: the thirty-day search experiments, SIMD prototypes, prod readbacks, and so on. It also adds references to external local paths (/Users/...) that other contributors can't open. This is a lot of review surface for a bug-fix PR, and it conflicts with the repo's "surgical changes" guidance. I'd split the docs into a separate PR and keep only the section on this fix and the MinIO selector here.
  • The PR is described as a draft, with the image unmerged and undeployed. Since a push to master deploys, make sure the make ci-signoff attestation corresponds to the final tree. The description cites 537ef2c1, which is the combined tree.

Performance and security

No concerns. The change only disables an optimization for non-timestamp sorts, and non-timestamp sorts no longer take the narrowing path they could never use correctly.

🤖 Generated with Claude Code

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