fix(ttl_map): initialize time to 1 for correct wrapping sub #224
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jayshrivastava
approved these changes
Nov 17, 2025
Collaborator
jayshrivastava
left a comment
jayshrivastava
left a comment
There was a problem hiding this comment.
Nit: I think adding a 1 sentence comment to sum up the PR description would be good here.
ruchirK
force-pushed
the
fix-time-wrapping-sub
branch
from
f6d9b25 to
09f9cd2
Compare
gabotechs
reviewed
Nov 18, 2025
Comment on lines
-187
to
+188
| time: Arc::new(AtomicU64::new(0)), | ||
| // Explicitly initialize `time` to 1 to avoid underflow issues with circular buffer. | ||
| time: Arc::new(AtomicU64::new(1)), |
Collaborator
There was a problem hiding this comment.
👍 nice. Any chance to reproduce this in a test that fails in main and succeeds in this branch?
Contributor
Author
There was a problem hiding this comment.
Ah that's a good q let me think on that for a minute
ruchirK
force-pushed
the
fix-time-wrapping-sub
branch
from
09f9cd2 to
063c1b4
Compare
gabotechs
approved these changes
Nov 19, 2025
Collaborator
gabotechs
left a comment
gabotechs
left a comment
There was a problem hiding this comment.
Nice! this is just missing to solve some clippy isues, but LGTM
Fixes datafusion-contrib#221. Previously, we encountered a bug at the initial gc tick on ttl_map where `time` (denoting the number of ticks since map initialization) starts at 0, but the logic to compute the time to remove the inserted value is ``` free_time = (time - 1) // (number of ticks to ttl) ``` In other words, we maintain a circular buffer of inserts, with a single slot per tick, and we will delete the item once `buffer.len()` ticks have elapsed (ttl = tick * buffer.len()). This is all fine, except we implement the logic as: ``` free_time = time.wrapping_sub(1) / buffer.len() ``` which is computing for `u64` `time`: ``` free_time = ((time - 1) mod 2^64) mod buffer.len() ``` when we really just want `free_time = (time - 1) mod buffer.len()`. Luckily this equality holds as long as ` 0 < time < 2^64`, as for those times `time - 1 mod 2&64 = time`. This commit changes our behavior to initialize `time` at 1 instead of 0. We don't need to worry about the overflow case because even if the tick duration was a nanosecond, it would take ~584 years to overflow 64 bits at which point we would surely have other problems besides the momentarily incorrect ttl. For the underflow case, this should primarily help with unit tests above anything else, as the bug only happened at `time = 0`.
ruchirK
force-pushed
the
fix-time-wrapping-sub
branch
from
063c1b4 to
5e5f7c0
Compare
| // Advance GC 3 times, which shouldn't free the first key. | ||
| for _ in 0..3 { | ||
| TTLMap::<String, i32>::gc(ttl_map.time.clone(), &ttl_map.buckets); | ||
| } |
Collaborator
There was a problem hiding this comment.
nit: I think we can just truncate everything after this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #221. Previously, we encountered a bug at the initial gc tick on ttl_map where
time(denoting the number of ticks since map initialization) starts at 0, but the logic to compute the time to remove the inserted value isIn other words, we maintain a circular buffer of inserts, with a single slot per tick, and we will delete the item once
buffer.len()ticks have elapsed (ttl = tick * buffer.len()).This is all fine, except we implement the logic as:
which is computing for
u64time:when we really just want
free_time = (time - 1) mod buffer.len().Luckily this equality holds as long as
0 < time < 2^64, as for those timestime - 1 mod 2&64 = time. This commit changes our behavior to initializetimeat 1 instead of 0. We don't need to worry about the overflow case because even if the tick duration was a nanosecond, it would take ~584 years to overflow 64 bits at which point we would surely have other problems besides the momentarily incorrect ttl. For the underflow case, this should primarily help with unit tests above anything else, as the bug only happened attime = 0.