fix(tiering): push measurement/time-range filters into SQL WHERE - #707
Conversation
|
All contributors have signed the CLA. Thanks! |
xe-nvdk
left a comment
There was a problem hiding this comment.
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:
- 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. - 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.mddescribing the fix, ending with the standard credit line you can copy from any neighboring entry.
|
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 |
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)
1407756 to
e5c2142
Compare
|
@pujitha24 awesome work! Thank you. You need to sign the CLA and we are ready to merge. |
xe-nvdk
left a comment
There was a problem hiding this comment.
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.
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 inthis 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