fix: preserve id type bits on update - #488
Conversation
WalkthroughThe record update path now stores nonzero KeyString type bits derived from ChangesRecord Update Type Bits
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
908-913: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fix; matches the insert path.
The rebuilt
idKeyStringand theSetUnpackInfo()call correctly mirror the primary-index logic already used in_insertRecords(lines 1152-1155). This closes the gap where updates previously dropped_idtype bits while inserts preserved them.The same 3-line "extract non-zero type bits and call
SetUnpackInfo" pattern now appears 4 times in this file (here, lines 955-957, 1153-1155, 1204-1206). Consider extracting a small helper to reduce duplication:♻️ Optional helper extraction
+namespace { +void setUnpackInfoFromTypeBits(Eloq::MongoRecord* record, const KeyString::TypeBits& typeBits) { + if (!typeBits.isAllZeros()) { + record->SetUnpackInfo(typeBits.getBuffer(), typeBits.getSize()); + } +} +} // namespaceThen each call site becomes, for example:
- if (const auto& typeBits = idKeyString.getTypeBits(); !typeBits.isAllZeros()) { - mongoRecord->SetUnpackInfo(typeBits.getBuffer(), typeBits.getSize()); - } + setUnpackInfoFromTypeBits(mongoRecord.get(), idKeyString.getTypeBits());🤖 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 `@src/mongo/db/modules/eloq/src/eloq_record_store.cpp` around lines 908 - 913, Extract the repeated non-zero type-bits extraction and SetUnpackInfo logic into a small shared helper, using the existing idKeyString/KeyString flow. Replace the duplicated blocks in the current update path and the corresponding sites near the other primary-index handling with calls to that helper, preserving the behavior of skipping all-zero type bits.
🤖 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.
Nitpick comments:
In `@src/mongo/db/modules/eloq/src/eloq_record_store.cpp`:
- Around line 908-913: Extract the repeated non-zero type-bits extraction and
SetUnpackInfo logic into a small shared helper, using the existing
idKeyString/KeyString flow. Replace the duplicated blocks in the current update
path and the corresponding sites near the other primary-index handling with
calls to that helper, preserving the behavior of skipping all-zero type bits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2222569c-7a6a-45c8-8c3d-6a97c340e6ab
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/eloq_record_store.cpptests/jstests/eloq_basic/update_index_validation.js
What changed
_idKeyString inEloqRecordStore::updateRecord().MongoRecord, matching the existing insert path.Why
KeyString value bytes normalize equivalent numeric representations, while TypeBits preserve their exact BSON types, such as
NumberLong, Double, Decimal, and negative zero.The insert path stored the
_idTypeBits inMongoRecord, but the update path replaced the record and wrote only the BSON payload. After an update, indexed queries could still return correct results because the physical index entries were maintained correctly. However,validate({full: true})reconstructed the record identity without the original TypeBits and could report_id_and every secondary index as invalid.This change preserves the
_idTypeBits during updates using the same encoding rule as inserts.Testing
Added
tests/jstests/eloq_basic/update_index_validation.js, covering:NumberIntas the all-zero TypeBits control caseNumberLongNumberLong_id_, unchanged secondary indexes, and updated secondary indexesreturnKey()Results:
NumberLongcase reproducedvalid: falsefor_id_,stable_1, andvalue_1, while hinted queries still returned the correct results.update_index_validation.jspassed: 1/1 tests.update_affects_indexes.jsandreturn_key.jsalso passed.git diff --checkpassed.Summary by CodeRabbit
Bug Fixes
_idtypes.Tests
_idvalues.