refactor(datacell): extract variable record layouts - #2737
Hidden character warning
Conversation
|
/label status/waiting-for-review |
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Require kind label
🟢 Require version label
|
There was a problem hiding this comment.
Pull request overview
This PR refactors variable-length record storage in the DataCell layer by introducing reusable layout primitives (explicit byte-range addressing plus policy-based variable record locations) and migrating SparseVectorDataCell / MultiVectorDataCell to use them while preserving existing on-disk schemas.
Changes:
- Added
ByteRangeLayoutfor explicit(offset, length)access over opaque bytes and IO delegation/serialization. - Added
VariableRecordLayoutcomposed ofFixedLayout(locations) +ByteRangeLayout(payload), with pluggable location policies. - Migrated
SparseVectorDataCellandMultiVectorDataCellto the new layout abstractions while keeping v2 + legacy serialization compatibility.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/layout/variable_record_layout.h | New policy-based variable-record layout (locations + payload) and location policy helpers. |
| src/layout/variable_record_layout_test.cpp | Unit tests validating record append, location mapping, and batched reads. |
| src/layout/byte_range_layout.h | New byte-range wrapper over BasicIO with offset/length read/write + serialization passthrough. |
| src/layout/byte_range_layout_test.cpp | Unit tests for range addressing and serialization round-trip. |
| src/datacell/sparse_vector_datacell.inl | Migrates sparse-vector code storage/query paths to VariableRecordLayout. |
| src/datacell/sparse_vector_datacell.h | Replaces bespoke packed location table + payload IO fields with VariableRecordLayout. |
| src/datacell/multi_vector_datacell.inl | Migrates multi-vector storage/query/serialization to VariableRecordLayout with header-length policy. |
| src/datacell/multi_vector_datacell.h | Replaces offset table + payload/current-offset fields with VariableRecordLayout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cbc490a to
9ec0f1b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
src/datacell/sparse_vector_datacell.inl:43
- The
load_locationlambda declaresDocLocation location{}but never uses it, which will trigger an unused-variable warning (and can break builds if warnings are treated as errors).
const auto load_location = [this](InnerIdType id) {
DocLocation location{};
return layout_.ReadLocation(id);
};
src/datacell/multi_vector_datacell.inl:218
- This comment still references
offset_io_, but the implementation now reads fromlayout_(locations table). Updating the comment will avoid confusion during future maintenance.
// Step 1: Read all offsets (offset_io_ is MemoryBlockIO, in-memory, fast)
std::vector<uint64_t> offsets(id_count);
for (InnerIdType i = 0; i < id_count; ++i) {
offsets[i] = layout_.ReadLocation(idx[i]);
Signed-off-by: LHT129 <tianlan.lht@antgroup.com> Assisted-by: Codex:GPT-5
9ec0f1b to
46d147f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/datacell/sparse_vector_datacell.inl:43
- The
load_locationlambda declaresDocLocation location{}but never uses it, which will trigger unused-variable warnings (and can fail builds if warnings are treated as errors). The lambda can just returnlayout_.ReadLocation(id)directly.
const auto load_location = [this](InnerIdType id) {
DocLocation location{};
return layout_.ReadLocation(id);
};
src/layout/byte_range_layout.h:75
ByteRangeLayout::Prefetchforwards its second argument toBasicIO::Prefetch(offset, cache_line), but the parameter is namedlength, which misrepresents what the value means. Renaming it tocache_line(and matching the default) makes the API clearer and consistent withBasicIO.
void
Prefetch(uint64_t offset, uint64_t length) {
io_->Prefetch(offset, length);
}
What
ByteRangeLayoutfor explicit(offset, length)access to opaque bytes.VariableRecordLayoutcomposed fromFixedLayoutandByteRangeLayout.SparseVectorDataCellwhile preserving the V2 and legacy location schemas.MultiVectorDataCellwhile preserving itsuint64_toffset table and serialized byte order.Design boundary
Layout owns addressing, byte organization, IO delegation, append allocation, and serialization primitives. It does not interpret quantized codes or perform distance computation.
Sparse query range merging remains in the DataCell because it coordinates result ordering and computation.
Tests