[dajiaohuang]

Fixes #3175.

Translate a caller's sparse view-space filter across the complete index mapping rather than stopping at its cardinality. Mapping source hits back also respects that filter. Preserve the original first-match behavior for duplicate source indices; this patch does not expand top-k hits into every duplicate view position.

Added real-dataset regressions in python/tests/test_filtered_column_query.py: an embedding column, reordered view [9, 2, 7], one-row filters at positions 1 and 2, and an unfiltered top-k ordering check. The tests execute nested dataset queries rather than copying the mapping algorithm.

Added a standalone native regression in cpp/tests/filtered_column_query_test.cpp with reproduction instructions in cpp/tests/README.md. It executes the actual filtered_column and array_column classes, controlling only the source index-search results. It explicitly calls the inline method because the public SDK owns an old vtable through an out-of-line key function. No mapping algorithm is copied. With the ABI-compatible 4.4.4 SDK, CRoaring 4.2.1 and Abseil 20240116.2, the old header fails three cases and passes two first-match controls; the PR header passes all five.

Validation: native class before/after checks, Black, clang-format and git diff --check pass. The three Python real-dataset tests pass against the published Linux 4.5.1 wheel, validating fixture/API only. The native class regression is isolated component evidence; it does not validate the query planner, actual index search, PostgreSQL integration or patched Python extension. The checkout supplies no Python extension build target, so full patched-extension validation remains outstanding.

Review follow-up

Removed the standalone native test files requested by review. Production changes are unchanged. The Python regression remains; native gtests need integration into cpp/tests/heimdall. Earlier isolated component checks are historical evidence only; full native integration and patched-extension acceptance remain unverified.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: bc9d820f-908b-44bf-bc15-6d12c8727dbc

📥 Commits

Reviewing files that changed from the base of the PR and between f432041 and 0693039.

📒 Files selected for processing (1)

• cpp/heimdall_common/impl/filtered_column.hpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The filtered-column top-k adapter now scans the full view when expanding a caller filter. It maps each source result to every matching filtered-view position that passes that filter.

Changes

Filtered top-k query mapping

Layer / File(s)	Summary
Filter expansion and result mapping cpp/heimdall_common/impl/filtered_column.hpp	Filter expansion checks every position in indices_. Result mapping emits all matching positions that pass the caller’s filter.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~7 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to 06930

Filtered top-k results preserve matching view positions, including sparse and repeated indices. No concrete merge-blocking issue is established.

Architecture Summary

Architecture risk: 🔵 Low · up to 06930

The change affects 1 system.

Changed systems: cpp

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — cpp (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in cpp/heimdall_common/impl/filtered_column.hpp: Filter expansion now checks every position in indices_; previously iteration was limited by the filter’s cardinality, which could skip matching positions.
• observed — Modified behavior in cpp/heimdall_common/impl/filtered_column.hpp: The result-mapping callback now captures the caller’s filter and emits every filtered position mapped to each source result that passes it. Previously it emitted only the first matching position, regardless of the caller’s filter.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #3175 requires each source result to map to every matching filtered-view position, to support repeated source indices, to scan sparse filtered-view positions, and to exclude caller-filtered posi…
Out of Scope Changes check	✅ Passed	The supplied change summary reports four additions and five deletions in cpp/heimdall_common/impl/filtered_column.hpp. These changes modify only the filtered_column::run_query mapping required by …
Title check	✅ Passed	The title clearly describes the main change: preserving filtered view positions in top-k results.
Description check	✅ Passed	The description is detailed and on-topic. It explains the fix, linked issue, regression coverage, validation results, and remaining validation limits. It does not reproduce the template's Impact secti…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checked each view-wide row,
And let allowed matches show.
Repeated paths came hopping through,
Each filtered spot was counted too.
The top-k basket held them all.}

_{Comment @coderabbitai help to get the list of available commands.}

[khustup2]

The bound fix is right: cardinality() is not an index bound, so a sparse filter was truncated. Two questions before this can go in:

1. Removing break changes behavior when indices_ repeats a source index. Show a case where a filtered view holds duplicate source indices. If no such case exists, keep the break.
2. Add a test that fails on the old code: a filter whose highest set bit is above its cardinality, and a top-k query over it. The test must run through the real column classes, not a reimplementation.

[dajiaohuang]

Restored break in 76f0bc3c: the source hit maps to the first eligible view position, preserving the existing duplicate/top-k behavior instead of expanding multiplicity. The sparse-filter fix still traverses the complete mapping and excludes ineligible duplicate positions during reverse mapping.

Added three real-column integration cases using an embedding column and a reordered [9, 2, 7] view. They cover one selected row at view positions 1 and 2 plus unfiltered top-k ordering; they call actual nested dataset queries and do not reproduce the C++ algorithm in Python. Black/diff checks pass. All three run against the published Linux 4.5.1 wheel, which confirms the fixture/API only. The wheel does not include this header change, so patched-extension validation remains outstanding and is explicitly stated in the description.

[dajiaohuang]

Added actual-class before/after coverage in 08b7062 (cpp/tests/filtered_column_query_test.cpp and cpp/tests/README.md). The old header fails the highest-bit-above-cardinality case, the multiple sparse-bit case, and the eligible duplicate case; with the PR header all five tests pass, including both first-match controls. Filtering and reverse mapping execute the real filtered_column class with an array_column source, not a copied algorithm. Only index-search results are controlled.

The standalone test explicitly qualifies the inline method with test-only access: ordinary virtual dispatch uses the old public SDK vtable, as confirmed by its R_X86_64_COPY relocation. The documented 4.4.4 SDK matches the class interface; 4.5.2 changes ABI types and cannot be mixed with this definition. This supplies native component evidence for the requested regression, while full patched Python-extension/query-planner/index integration remains unverified. The existing three nested real-dataset tests remain separate integration coverage.

[khustup2]

Source change approved: the cardinality bound was wrong (cardinality is not an index bound) and the filter-aware reverse mapping with break restored keeps the first-match behavior. The regression test proves the point but cannot land as written: #define private public and a standalone main at the cpp/tests root are outside our harness, and the vtable workaround is a symptom of testing against a prebuilt SDK rather than the tree. We will carry the fix with a gtest in cpp/tests/heimdall against the source.