Skip to content

fix(tiering): push measurement/time-range filters into SQL WHERE - #707

Merged
xe-nvdk merged 2 commits into
Basekick-Labs:mainfrom
pujitha24:auto/issue-346
Sep 11, 2026
Merged

fix(tiering): push measurement/time-range filters into SQL WHERE#707
xe-nvdk merged 2 commits into
Basekick-Labs:mainfrom
pujitha24:auto/issue-346

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

Motivation:
Router.GetStoragePathsForQuery fetched every file recorded for a
database via MetadataStore.GetFilesByDatabase, then filtered by
measurement and time range in a Go loop. On a database with many
measurements or a long history, this pulls every row (and every
unrelated measurement's files) out of SQLite before discarding most
of them in application code, instead of letting SQLite's existing
database/tier index prune rows before they cross the query boundary.

Approach:
Added MetadataStore.GetFilesForQuery(ctx, database, measurement,
startTime, endTime), which builds the WHERE clause dynamically:
database is always filtered, measurement is added only when
non-empty, and partition_time bounds are added only when the
corresponding pointer is non-nil. All values are bound via
placeholders. The boundary semantics match the removed Go code
exactly (partition_time >= start, partition_time <= end, both
inclusive). Router.GetStoragePathsForQuery now calls this method
and only groups the already-filtered rows by tier. GetFilesByDatabase
itself is untouched; its other callers (the admin files-listing API,
the unrelated Raft FSM in-memory method) don't filter and were out
of scope for this issue.

Validation:
go build ./internal/tiering/...
go vet -tags=duckdb_arrow ./internal/tiering/...
go test -tags=duckdb_arrow -race ./internal/tiering/...
All three passed, including the new TestMetadataStore_GetFilesForQuery,
which covers measurement filtering, cross-database exclusion, the
inclusive time-range boundary, and the empty-measurement fallback.
A full-repo go build -tags=duckdb_arrow ./... could not complete in
this environment (host disk ran out of space partway through fetching
unrelated module dependencies) — this is an environment limitation
unrelated to the change, and the affected package's own build, vet,
and race-enabled tests all pass cleanly.

No user-visible behavior changes: GetStoragePathsForQuery returns the
same set of paths as before. The benefit is avoiding an unnecessary
full-database metadata scan and reducing what a busy tiering database
holds in memory per query.

Report: #346
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
Assisted-by: claude-sonnet-5 (via Claude Code)

Fixes #346

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA. Thanks!
Posted by the CLA Assistant Lite bot.

@xe-nvdk xe-nvdk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for taking #346, and for the unusually clear PR description; it made the review a pleasure. The code is correct and I will merge once the two administrative items at the end are done.

What I verified:

Semantic parity with the removed Go filter. The one real risk in this kind of pushdown is time-comparison drift: SQLite compares TIMESTAMP columns lexicographically as strings, so correctness depends on uniform serialization. It holds here: RecordFile is the only insert path and binds PartitionTime.UTC(), your query binds .UTC() bounds, and the driver's trailing-zero-trimmed fractional format still orders chronologically for same-zone values (the existing GetFilesOlderThan already relies on this). To be sure, I ran a differential probe on your branch: 200 files with random sub-second partition times, 300 randomized queries mixing nil bounds, exact-boundary hits, and non-UTC query timezones, asserting the old GetFilesByDatabase plus in-Go filter and your GetFilesForQuery return identical sets. They do.

Ordering and shape. ORDER BY partition_time DESC matches what GetFilesByDatabase already returned, so downstream tier grouping sees the same order; the SELECT column list matches scanFiles; all values go through placeholders.

Scope claim. Confirmed: the remaining GetFilesByDatabase callers (the admin files-listing API and the Raft FSM's unrelated in-memory method) don't filter, so leaving them untouched is right.

Suite health. Full -tags=duckdb_arrow build and vet across the repo (covering the environment limitation you mentioned), plus the tiering package under -race, all green.

Blocking for merge, both quick:

  1. CLA. The CLA check is failing; signing is required on every PR. Please sign per the bot's instructions in this thread and comment recheck.
  2. Release notes. Community fixes get an entry in the current planned release file. Please add a short entry near the top of the Bug fixes section in RELEASE_NOTES_2026.09.2.md describing the fix, ending with the standard credit line you can copy from any neighboring entry.

@xe-nvdk

xe-nvdk commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thank you again for this contribution — the pushdown is exactly how we wanted #346 solved, and the differential testing made it easy to trust.

To save you a round trip, I have pushed the release-notes entry (with your credit line) to your branch, so that item from my review is covered. The only thing left is the CLA: please sign it per the bot's instructions in this thread and comment recheck. Once the check flips green this merges.

pujitha24 and others added 2 commits September 11, 2026 03:00
Motivation:
Router.GetStoragePathsForQuery fetched every file recorded for a
database via MetadataStore.GetFilesByDatabase, then filtered by
measurement and time range in a Go loop. On a database with many
measurements or a long history, this pulls every row (and every
unrelated measurement's files) out of SQLite before discarding most
of them in application code, instead of letting SQLite's existing
database/tier index prune rows before they cross the query boundary.

Approach:
Added MetadataStore.GetFilesForQuery(ctx, database, measurement,
startTime, endTime), which builds the WHERE clause dynamically:
database is always filtered, measurement is added only when
non-empty, and partition_time bounds are added only when the
corresponding pointer is non-nil. All values are bound via
placeholders. The boundary semantics match the removed Go code
exactly (partition_time >= start, partition_time <= end, both
inclusive). Router.GetStoragePathsForQuery now calls this method
and only groups the already-filtered rows by tier. GetFilesByDatabase
itself is untouched; its other callers (the admin files-listing API,
the unrelated Raft FSM in-memory method) don't filter and were out
of scope for this issue.

Validation:
go build ./internal/tiering/...
go vet -tags=duckdb_arrow ./internal/tiering/...
go test -tags=duckdb_arrow -race ./internal/tiering/...
All three passed, including the new TestMetadataStore_GetFilesForQuery,
which covers measurement filtering, cross-database exclusion, the
inclusive time-range boundary, and the empty-measurement fallback.
A full-repo `go build -tags=duckdb_arrow ./...` could not complete in
this environment (host disk ran out of space partway through fetching
unrelated module dependencies) — this is an environment limitation
unrelated to the change, and the affected package's own build, vet,
and race-enabled tests all pass cleanly.

No user-visible behavior changes: GetStoragePathsForQuery returns the
same set of paths as before. The benefit is avoiding an unnecessary
full-database metadata scan and reducing what a busy tiering database
holds in memory per query.

Report: Basekick-Labs#346
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: claude-sonnet-5 (via Claude Code)
@xe-nvdk

xe-nvdk commented Sep 11, 2026

Copy link
Copy Markdown
Member

@pujitha24 awesome work! Thank you. You need to sign the CLA and we are ready to merge.

@xe-nvdk xe-nvdk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CLA is registered now, so this is clear to merge.

Re-verified on the current head: the tiering code is byte-identical to what I reviewed, so the differential probe result stands (200 randomized files against 300 randomized queries, comparing the removed Go filter with the new SQL pushdown, including nil bounds, exact-boundary hits and non-UTC query zones, identical sets throughout). Nothing in main's internal/tiering has moved since. It merges cleanly with current main, both release-notes entries survive with no upstream headings lost, and the tiering suite is green under -race on the merged state.

Thanks for the clean pushdown and for the patience through the CLA snag.

@xe-nvdk
xe-nvdk merged commit 0797d94 into Basekick-Labs:main Sep 11, 2026
2 of 3 checks passed
xe-nvdk added a commit that referenced this pull request Sep 11, 2026
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.

medium(tiering): GetStoragePathsForQuery filters client-side

2 participants