fix:ttl#3244
Conversation
📝 WalkthroughWalkthroughThe changes update timestamp parsing logic in two storage format headers. Instead of equality-based detection, the code now explicitly checks a high-bit flag (bit 63) to determine whether timestamps are stored in milliseconds or seconds before applying conversion. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/src/strings_value_format.h (1)
52-69:⚠️ Potential issue | 🔴 Critical
HyperloglogValue::Encodedoes not set the milliseconds flag bit — timestamps will be corrupted when decoded.
StringsValue::Encodenow ORs1ULL << 63into non-zeroctime_/etime_before writing, andParsedStringsValuetreats any decoded timestamp with bit 63 == 0 as a legacy seconds value and multiplies it by 1000. However,HyperloglogValue::Encodewrites rawctime_/etime_here (lines 65, 67) without the high-bit flag. SinceHyperloglogValuesharesDataType::kStringsand is decoded throughParsedStringsValue, any non-zero hyperloglog ctime/etime will be wrongly scaled by 1000 on read, producing bogus expirations. The two encoders must agree on the on-disk format.🔧 Suggested fix
memcpy(dst, reserve_, kSuffixReserveLength); dst += kSuffixReserveLength; - EncodeFixed64(dst, ctime_); + // Match StringsValue: high bit = 1 indicates a millisecond timestamp. + uint64_t ctime = ctime_ > 0 ? (ctime_ | (1ULL << 63)) : 0; + EncodeFixed64(dst, ctime); dst += kTimestampLength; - EncodeFixed64(dst, etime_); + uint64_t etime = etime_ > 0 ? (etime_ | (1ULL << 63)) : 0; + EncodeFixed64(dst, etime); return {start_, needed};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/strings_value_format.h` around lines 52 - 69, HyperloglogValue::Encode writes raw ctime_/etime_ but must set the millisecond/high-bit flag before calling EncodeFixed64 so ParsedStringsValue won't treat them as legacy-second timestamps; update HyperloglogValue::Encode to OR each non-zero timestamp (ctime_ and etime_) with the high-bit mask (1ULL << 63 or the project constant used by StringsValue) prior to the EncodeFixed64 calls for ctime_ and etime_, leaving reserve_[0] handling as-is.
🧹 Nitpick comments (3)
src/storage/src/base_meta_value_format.h (2)
82-90: Consider extracting the decode logic into a helper.The same "mask bit 63; if bit was clear multiply by 1000" pattern is now duplicated four times across the two
ParsedBaseMetaValueconstructors and the twoParsedStringsValueconstructors. A small inline helper (e.g.static inline uint64_t DecodeTimestamp(uint64_t raw) { return (raw & (1ULL << 63)) ? (raw & ~(1ULL << 63)) : raw * 1000; }) would remove the duplication, avoid the subtle two-stepctime_ = …; if (…) ctime_ *= 1000;sequence, and keep the seconds-vs-ms decoding rule in a single place alongside the encode-side| (1ULL << 63)logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/base_meta_value_format.h` around lines 82 - 90, Extract the repeated "mask bit 63; if bit clear multiply by 1000" logic into a single helper (e.g. static inline uint64_t DecodeTimestamp(uint64_t raw)) and replace the duplicated sequences in the ParsedBaseMetaValue constructors and ParsedStringsValue constructors with calls to that helper; ensure the helper returns (raw & ~(1ULL << 63)) when the top bit is set and returns raw*1000 when the top bit is clear so the decoding rule is centralized and the current two-step assign/then-multiply pattern is eliminated.
112-125: Replace Chinese inline comments with English for consistency.The rest of this header (and the codebase file header) is in English; the newly added comments on lines 112-113, 116, 119, 122, 125 are in Chinese. Please translate them to English (the equivalent wording already exists in
Encode()at lines 42-43). Also, the duplicated// 秒时间戳，转换为毫秒inside bothifbranches is redundant given the outer comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/base_meta_value_format.h` around lines 112 - 125, Replace the Chinese inline comments around the timestamp parsing logic in the function that sets ctime_ and etime_ with English equivalents for consistency (match the wording used in Encode()); specifically update the comments on the bit-strip and check operations for ctime_ and etime_ (the lines manipulating ctime, etime, (1ULL << 63), and assignments to ctime_ and etime_) to English and remove the duplicate “// 秒时间戳，转换为毫秒” comment inside both if branches, keeping one clear English comment that states: strip the MSB flag and if MSB==0 treat as seconds and multiply by 1000 to convert to milliseconds (same wording as Encode()).src/storage/src/strings_value_format.h (1)
115-127: Translate Chinese comments to English and drop the redundant inner comments.Mirror comment in
base_meta_value_format.h: please use English for consistency with the rest of this file, and the inner// 秒时间戳，转换为毫秒duplicates the header comment — one comment above the two blocks is enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/src/strings_value_format.h` around lines 115 - 127, Replace the Chinese comments around the timestamp parsing in strings_value_format.h with a single English comment and remove the redundant inner comments: update the header comment above the two blocks that manipulate ctime_/etime_ (and use ctime/etime and the high-bit check logic) to an English line like "Fix timestamp parsing: treat value as milliseconds unless high bit is cleared indicating seconds; strip high bit and convert seconds to milliseconds", then delete the duplicated inner comments that currently read "秒时间戳，转换为毫秒" so only the new English header comment remains; ensure the logic in the ctime_/etime_ blocks (bit-strip and conditional multiply by 1000) is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/storage/src/strings_value_format.h`:
- Around line 52-69: HyperloglogValue::Encode writes raw ctime_/etime_ but must
set the millisecond/high-bit flag before calling EncodeFixed64 so
ParsedStringsValue won't treat them as legacy-second timestamps; update
HyperloglogValue::Encode to OR each non-zero timestamp (ctime_ and etime_) with
the high-bit mask (1ULL << 63 or the project constant used by StringsValue)
prior to the EncodeFixed64 calls for ctime_ and etime_, leaving reserve_[0]
handling as-is.
---
Nitpick comments:
In `@src/storage/src/base_meta_value_format.h`:
- Around line 82-90: Extract the repeated "mask bit 63; if bit clear multiply by
1000" logic into a single helper (e.g. static inline uint64_t
DecodeTimestamp(uint64_t raw)) and replace the duplicated sequences in the
ParsedBaseMetaValue constructors and ParsedStringsValue constructors with calls
to that helper; ensure the helper returns (raw & ~(1ULL << 63)) when the top bit
is set and returns raw*1000 when the top bit is clear so the decoding rule is
centralized and the current two-step assign/then-multiply pattern is eliminated.
- Around line 112-125: Replace the Chinese inline comments around the timestamp
parsing logic in the function that sets ctime_ and etime_ with English
equivalents for consistency (match the wording used in Encode()); specifically
update the comments on the bit-strip and check operations for ctime_ and etime_
(the lines manipulating ctime, etime, (1ULL << 63), and assignments to ctime_
and etime_) to English and remove the duplicate “// 秒时间戳，转换为毫秒” comment inside
both if branches, keeping one clear English comment that states: strip the MSB
flag and if MSB==0 treat as seconds and multiply by 1000 to convert to
milliseconds (same wording as Encode()).
In `@src/storage/src/strings_value_format.h`:
- Around line 115-127: Replace the Chinese comments around the timestamp parsing
in strings_value_format.h with a single English comment and remove the redundant
inner comments: update the header comment above the two blocks that manipulate
ctime_/etime_ (and use ctime/etime and the high-bit check logic) to an English
line like "Fix timestamp parsing: treat value as milliseconds unless high bit is
cleared indicating seconds; strip high bit and convert seconds to milliseconds",
then delete the duplicated inner comments that currently read "秒时间戳，转换为毫秒" so
only the new English header comment remains; ensure the logic in the
ctime_/etime_ blocks (bit-strip and conditional multiply by 1000) is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f58f1c1e-768e-4fd2-a424-a31cccbc78c0
📒 Files selected for processing (2)
src/storage/src/base_meta_value_format.hsrc/storage/src/strings_value_format.h
2 participants
Summary by CodeRabbit
Release Notes