feat(search): composite token+quantity search via dedicated table#29
feat(search): composite token+quantity search via dedicated table#29nirmal070125 wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds a dedicated composite token-quantity table and indexes, materializes paired values during extraction, and removes them during deletion. Search generation now uses the table for token and quantity predicates with direct-drive support. Planner statistics, autovacuum, and tenant RLS configuration are updated. Tests cover matching semantics, comparators, token and unit forms, lifecycle updates, SQL generation, and index plans. Sequence Diagram(s)sequenceDiagram
participant SearchParams
participant Extractor
participant PostgreSQL
participant queryBuilder
SearchParams->>Extractor: index resource composite values
Extractor->>PostgreSQL: store token-quantity pairs
SearchParams->>queryBuilder: submit composite search
queryBuilder->>PostgreSQL: query composite predicates and indexes
PostgreSQL-->>SearchParams: return matching resources
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/store/search_idfirst_test.go (1)
292-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a genuinely nested token+quantity composite case.
The remaining cases only exercise nested plain quantities. Add a chained or
_hascomposite case that verifiesnumericTablestays empty, covering the composite path’s separate direct-drive suppression logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/store/search_idfirst_test.go` around lines 292 - 299, Add a test case to TestNestedNumericNotDirectDrive using a chained target or _has key whose value is a token+quantity composite, and assert that numericTable remains empty. Keep the existing plain-quantity cases and verify the composite path’s direct-drive suppression separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/schema.sql`:
- Around line 660-677: Do not mark the built-in numrange constructor LEAKPROOF
globally. Introduce a dedicated wrapper for the bounded quantity range
construction, mark only that wrapper leakproof after preserving the required
behavior, and update both the idx_sp_qty_range_gist index expression and its RLS
predicate to use it; otherwise leave the range overlap as a post-filter.
- Around line 4-6: Update the schema prerequisite comment near resource_json to
state that PostgreSQL must be built with --with-lz4 for COMPRESSION lz4 support,
or change the compression setting to pglz/default if that requirement should not
be imposed.
In `@internal/store/search_composite_plan_test.go`:
- Around line 94-99: Update the bounded gePlan assertion in the search composite
plan test to require the exact GiST index name idx_sp_comp_tokqty_range_gist,
replacing the broader idx_sp_comp_tokqty substring check while preserving the
existing failure message and plan logging.
In `@internal/store/search.go`:
- Line 609: Validate quantity components in compositeQtyCond and
buildNumberExists before constructing predicates: propagate ParseFloat errors
and reject non-finite values instead of defaulting them to zero. Update their
callers, including the flow assigning qtyCond, to return or handle these errors
while preserving normal finite-value behavior.
In `@internal/store/store_integration_test.go`:
- Around line 776-789: Check and handle the error returned by each setup call to
s.Create in the composite-test setup: the same-element Observation creation at
internal/store/store_integration_test.go:776-789, every comparator Observation
creation at 811-817, and the token/unit Observation creation at 847-851. Fail
the test immediately on any creation error so setup failures are reported
directly rather than as misleading search-count failures.
---
Nitpick comments:
In `@internal/store/search_idfirst_test.go`:
- Around line 292-299: Add a test case to TestNestedNumericNotDirectDrive using
a chained target or _has key whose value is a token+quantity composite, and
assert that numericTable remains empty. Keep the existing plain-quantity cases
and verify the composite path’s direct-drive suppression separately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73846a79-bec5-41c6-b6d9-10c67d0f09ce
📒 Files selected for processing (7)
internal/db/schema.sqlinternal/index/extractor.gointernal/store/search.gointernal/store/search_composite_plan_test.gointernal/store/search_idfirst_test.gointernal/store/search_paramsweep_test.gointernal/store/store_integration_test.go
| // Bounded ge search: the GiST range-overlap index must be reachable. | ||
| gePlan := plan("8867-4$ge60") | ||
| t.Logf("ge plan:\n%s", gePlan) | ||
| if !strings.Contains(gePlan, "idx_sp_comp_tokqty") { | ||
| t.Errorf("ge: expected a composite index scan (GiST for overlap), got:\n%s", gePlan) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Assert the specific GiST index name.
This currently accepts any idx_sp_comp_tokqty* index, so it would not detect the overlap predicate falling back to another index plus filtering. Check for idx_sp_comp_tokqty_range_gist explicitly.
Proposed change
- if !strings.Contains(gePlan, "idx_sp_comp_tokqty") {
+ if !strings.Contains(gePlan, "idx_sp_comp_tokqty_range_gist") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Bounded ge search: the GiST range-overlap index must be reachable. | |
| gePlan := plan("8867-4$ge60") | |
| t.Logf("ge plan:\n%s", gePlan) | |
| if !strings.Contains(gePlan, "idx_sp_comp_tokqty") { | |
| t.Errorf("ge: expected a composite index scan (GiST for overlap), got:\n%s", gePlan) | |
| } | |
| // Bounded ge search: the GiST range-overlap index must be reachable. | |
| gePlan := plan("8867-4$ge60") | |
| t.Logf("ge plan:\n%s", gePlan) | |
| if !strings.Contains(gePlan, "idx_sp_comp_tokqty_range_gist") { | |
| t.Errorf("ge: expected a composite index scan (GiST for overlap), got:\n%s", gePlan) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/search_composite_plan_test.go` around lines 94 - 99, Update
the bounded gePlan assertion in the search composite plan test to require the
exact GiST index name idx_sp_comp_tokqty_range_gist, replacing the broader
idx_sp_comp_tokqty substring check while preserving the existing failure message
and plan logging.
There was a problem hiding this comment.
Addressed, but not by pinning the index name — that would be flaky. On small data the planner legitimately serves ge via the recency index (idx_sp_comp_tokqty_recent) rather than the range-GiST; both are composite indexes. Instead the test now also asserts the range-overlap operator && appears in the plan, so a scalar-bound fallback that drops the overlap is still caught.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
test_file="$(fd -t f '^search_composite_plan_test\.go$' internal/store | head -n1)"
search_file="$(fd -t f '^search\.go$' internal/store | head -n1)"
printf '%s\n' '--- plan test ---'
sed -n '80,120p' "$test_file"
printf '%s\n' '--- composite quantity predicate locations ---'
rg -n -A35 -B8 'func \(.*\) compositeQtyCond|numrange\(.*&&|buildCompositeTokenQuantityExists' "$search_file"Length of output: 9249
@nirmal070125, confirmed from the current code: compositeQtyCond emits the && overlap predicate for ge, and the updated plan test checks for that predicate while allowing a valid composite-index choice. The revised assertion is appropriate.
9fc8a78 to
3b24490
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/db/schema.sql (2)
4-6: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueDocument the LZ4 build requirement.
PostgreSQL 14+ alone does not guarantee
COMPRESSION lz4is available. Update the comment to explicitly call out the--with-lz4build requirement, or change the compression setting topglzordefaultif this requirement should not be imposed.📝 Proposed comment update
--- Requires PostgreSQL 14+ (resource_json uses COMPRESSION lz4). For Location +-- Requires PostgreSQL 14+ (built with --with-lz4 for COMPRESSION lz4). For Location -- near-search, install PostGIS. The btree_gist extension is created by this🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/schema.sql` around lines 4 - 6, Update the header comment in schema.sql to state that PostgreSQL must be built with --with-lz4 for the resource_json compression setting to work; alternatively, change that setting to pglz or default if an LZ4 build requirement is not intended.
660-677: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid marking the built-in
numrangeconstructorLEAKPROOFglobally.
LEAKPROOFis a function-wide security contract. The built-innumrangeconstructor can raise argument-dependent errors outside the indexed path. Introduce a dedicated wrapper for bounded quantity range construction, mark only the wrapper asLEAKPROOF, and update both the index expressions and Row-Level Security predicates to use it. If a wrapper is not implemented, leave the range overlap as a post-filter.As per path instructions, this validates safety using neutral, high-level language without exposing sensitive context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/schema.sql` around lines 660 - 677, The index path must not mark the built-in numrange constructor LEAKPROOF globally. Add a dedicated bounded-quantity range wrapper, mark only that wrapper LEAKPROOF, and replace direct numrange usage in idx_sp_qty_range_gist and the related Row-Level Security predicates with it; otherwise keep range overlap as a post-filter.Source: Path instructions
🧹 Nitpick comments (1)
internal/db/schema.sql (1)
328-330: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInclude trailing columns to ensure index-only EXISTS probes.
To fulfill the documented intent of the
_sourceindexes (resolving per-resource probes without heap-fetching), include thesystem,qty_system, andqty_codecolumns. Without these, composite queries filtering on the token system or quantity system/unit will require a heap fetch during the correlated scan.⚡ Proposed index refinement
-- Per-resource source probe + reindex DELETE / FK cascade support. CREATE INDEX IF NOT EXISTS idx_sp_comp_tokqty_source ON sp_composite_token_quantity - (tenant_id, resource_id, resource_type, param_name, code, value_low, value_high); + (tenant_id, resource_id, resource_type, param_name, code, value_low, value_high) + INCLUDE (system, qty_system, qty_code);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/schema.sql` around lines 328 - 330, Update the idx_sp_comp_tokqty_source index definition on sp_composite_token_quantity to include trailing system, qty_system, and qty_code columns, preserving the existing leading-column order so per-resource EXISTS probes can be satisfied index-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@internal/db/schema.sql`:
- Around line 4-6: Update the header comment in schema.sql to state that
PostgreSQL must be built with --with-lz4 for the resource_json compression
setting to work; alternatively, change that setting to pglz or default if an LZ4
build requirement is not intended.
- Around line 660-677: The index path must not mark the built-in numrange
constructor LEAKPROOF globally. Add a dedicated bounded-quantity range wrapper,
mark only that wrapper LEAKPROOF, and replace direct numrange usage in
idx_sp_qty_range_gist and the related Row-Level Security predicates with it;
otherwise keep range overlap as a post-filter.
---
Nitpick comments:
In `@internal/db/schema.sql`:
- Around line 328-330: Update the idx_sp_comp_tokqty_source index definition on
sp_composite_token_quantity to include trailing system, qty_system, and qty_code
columns, preserving the existing leading-column order so per-resource EXISTS
probes can be satisfied index-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a7c7ef8-8bb5-4952-a3d2-ae7bb8e9c514
📒 Files selected for processing (7)
internal/db/schema.sqlinternal/index/extractor.gointernal/store/search.gointernal/store/search_composite_plan_test.gointernal/store/search_idfirst_test.gointernal/store/search_paramsweep_test.gointernal/store/store_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/store/store_integration_test.go
- internal/store/search.go
- internal/store/search_idfirst_test.go
- internal/index/extractor.go
3b24490 to
28dafb8
Compare
Add sp_composite_token_quantity, a tenant-scoped table holding token+quantity composite pairs (code-value-quantity, component-code-value-quantity, ...) materialised at index time — one row per composite param per element pairing where both components co-occur in the SAME element. Token+quantity composite searches route to this single table with one index scan; all other sp_* tables and non-composite paths are unchanged. This also fixes a spec-conformance bug: the legacy path correlated the two components only at the resource level, so a multi-component Observation could cross-match component A's code with component B's value. Per-element rows make same-element pairing the only match. Schema: new table + code_value / recency / range-GiST / source indexes, planner statistics, autovacuum tuning, RLS policy. The GiST predicate stays byte-for-byte identical to buildQuantityExists so the range-overlap index is reachable; existing numrange/range_overlaps LEAKPROOF markings cover it. Indexer: dual-writes composite rows alongside the component sp_token/sp_quantity rows, pairing token codings with the quantity value within each element only; reindex DELETE and FK cascade include the new table. Query path: token+quantity composites reuse the existing direct-drive machinery so the planner chooses between the value-driven (code_value / GiST) and recency-driven plan shapes. Tests: same-element false-positive regression, all comparators with boundaries, token system|code / unit-scoped forms, update/delete reindex, and an EXPLAIN plan check that the composite indexes are used.
28dafb8 to
1f63f1b
Compare
ab2c47a to
1a65444
Compare
Verify and document compatibility with PostgreSQL 18. The schema (lz4 COMPRESSION, btree_gist, GiST numrange/tstzrange, RLS + LEAKPROOF, extended statistics) and all queries run unchanged on 18. - Test containers (testutil.PostgresImage + conformance harness) default to postgres:18-alpine and are overridable via FHIR_TEST_POSTGRES_IMAGE so the whole supported range (14 through 18) stays testable in one command. - docker-compose.yml bumped to postgres:18-alpine. - Docs state the supported range as PostgreSQL 14+ (tested through 18); the stale "13+" floor is corrected to 14 (the actual floor — resource_json uses lz4 COMPRESSION, which requires 14+). Verified: full integration suite and the FHIR R4 conformance suite pass on PostgreSQL 18; schema DDL, force_custom_plan, and the range-overlap plan tests also pass on the 14 floor via FHIR_TEST_POSTGRES_IMAGE=postgres:14-alpine. (cherry picked from commit d09f07f)
1a65444 to
9a5bc97
Compare
Half-bounded quantity comparators had no seekable access path: ge/le emitted a numrange && half-open probe that overlapped nearly every GiST leaf, and gt/lt rode a scalar on the wrong bound column, so every plan degraded to a full scan of the (tenant, type, param) partition. On the perf dataset value-quantity=ge99999 was the slowest query (~800ms p_max). Emit the collapsed scalar predicate on the correct bound column and split the comparators into two index-served families: value_high-driven: ge (>=), gt (>), eb (<) -> idx_sp_qty_high (new) value_low-driven: le (<=), lt (<), sa (>) -> idx_sp_qty_raw (existing) eq/ne/ap keep the numrange && overlap byte-for-byte so windowed queries stay on the GiST index. sa/eb are now handled explicitly instead of falling through to eq. Emitting on the correct column preserves straddling correctness: a stored [50, 100050] matches ge99999 via value_high and lt100 via value_low. The search band is quantized to the value column scale (DECIMAL(20,6)) so the strict comparators stay boundary-exact: a stored point value's value_high/value_low is rounded to 6dp on write, and comparing against an unrounded search edge would leak a boundary value in (gt65 matching a stored 65 whose value_high rounds to 65.000007). Schema: add idx_sp_qty_high and idx_sp_comp_tokqty_code_high covering value_high after the equality prefix, and raise statistics targets on value_low/value_high (1000 buckets) so the planner's per-column histogram distinguishes sparse high-tail bounds from dense ones and picks the value-driven seek vs. the recency walk accordingly. Verified with EXPLAIN (ANALYZE, BUFFERS): ge99999 rides an Index Only Scan on idx_sp_qty_high with value_high as an Index Cond (buffers scale with the match set, not the partition); ge0 switches to the recency walk; lt5 stays on idx_sp_qty_raw. All unit and integration tests pass.
|
Added commit Problem: half-bounded comparators had no seekable access path. Change:
Verified with The k6 p_max/TPS numbers and the ingest write-amplification check (design §6.4–5) still need a run on the perf environment. |
1 participant
What
Token+quantity composite searches (
code-value-quantity,component-code-value-quantity, …) currently walksp_tokennewest-first and run a correlatedEXISTSagainstsp_quantityfor each candidate, which can't early-exit when the intersection is sparse (a common code, a rare value). This adds a dedicatedsp_composite_token_quantitytable that holds the token+quantity pairs materialised at index time, so those searches resolve with a single index scan.It also fixes a correctness bug. FHIR composite semantics require both components to match within the same element, but the resource-level
EXISTScorrelation let a multi-component Observation match component A's code against component B's value. Storing one row per element pairing makes same-element matching the only match.How
sp_composite_token_quantitytable with code/value btree, recency, range-GiST, and per-resource source indexes, plus planner statistics, autovacuum tuning, and the tenant RLS policy. The GiST predicate is byte-for-byte identical to the quantity path so the range-overlap index stays reachable.sp_token/sp_quantityrows, pairing each element's token codings with that element's quantity value only. Update/delete re-index and the FK cascade cover the new table.system|code/ bare-code tokens and unit-scoped quantities; other composite types keep the current path.Tests
Same-element regression, all six comparators with boundary cases, token and unit-scoped forms, update/delete re-index, a SQL-shape unit test, and an
EXPLAINcheck that the composite indexes are used.