Fix float precision loss in datetime_to_microseconds - #1310
Conversation
int(dt.timestamp() * 1_000_000) truncates a float64 that has already accumulated rounding error from the multiplication - off by one microsecond for a large fraction of timestamps (~35% in random testing), and off by a full second for dates far enough from 1970 (e.g. year 9999) that the whole-seconds part alone exceeds what float64 can represent alongside the fractional part. This value feeds ORDER BY comparisons in local mode (Filter conditions, scroll ordering), so an off-by-one microsecond can flip a boundary comparison and disagree with what the real Qdrant server would return for the same query - a local/remote congruence bug. Fixed by routing through datetime subtraction instead of timestamp(): subtracting two aware datetimes is exact integer arithmetic internally, no floats involved. A naive input is first attached to the system's local timezone via astimezone() - the same "assume local time" behavior timestamp() has for naive input - which itself returns a datetime, not a lossy float, so precision holds all the way through. Added tests/test_order_by.py: cross-checks against an independent exact-arithmetic reference implementation, including the year-9999 case that the first fix attempt (only replacing the multiplication, still using timestamp() for the whole-seconds part) still got wrong. Verified failing against the original implementation before the fix.
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change replaces lossy timestamp conversion with exact datetime arithmetic for microsecond ordering; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_order_by.py (1)
6-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the test oracle independent of
datetime_to_microseconds.
_exact_microseconds_since_epochuses the same epoch-subtraction and delta-component expression asqdrant_client/local/order_by.py. Use a second independent integer implementation for the expected value instead.🤖 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 `@tests/test_order_by.py` around lines 6 - 11, Update the test helper _exact_microseconds_since_epoch to calculate expected microseconds using an independent integer-based formulation rather than the epoch-subtraction and timedelta component expression shared with datetime_to_microseconds. Keep the helper exact and preserve its existing return contract for all tested datetimes.
🤖 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.
Inline comments:
In `@qdrant_client/local/order_by.py`:
- Around line 23-25: Update datetime_to_microseconds so it treats a datetime as
naive when either dt.tzinfo is None or dt.utcoffset() is None, normalizing both
cases with astimezone() before subtracting _EPOCH. Add a regression test
covering a tzinfo implementation whose utcoffset() returns None.
---
Nitpick comments:
In `@tests/test_order_by.py`:
- Around line 6-11: Update the test helper _exact_microseconds_since_epoch to
calculate expected microseconds using an independent integer-based formulation
rather than the epoch-subtraction and timedelta component expression shared with
datetime_to_microseconds. Keep the helper exact and preserve its existing return
contract for all tested datetimes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0b484fa-8db1-4ffe-8105-ac8fc3ae7b5c
📒 Files selected for processing (2)
qdrant_client/local/order_by.pytests/test_order_by.py
shashvat-singham
left a comment
shashvat-singham
left a comment
There was a problem hiding this comment.
Ran the branch. The precision fix itself checks out — cross-checking both implementations against exact integer arithmetic over 20k random timestamps in the 0–2e15µs range, the old int(dt.timestamp() * MICROS_PER_SECOND) is off by a microsecond on ~1.3% of them, and the new version matches exactly everywhere I tried, including pre-1970 dates where timedelta normalisation makes days negative and seconds/microseconds positive.
Two things:
1. The comment overstates the far-future case. It says the old code is "off by a full second for dates far enough from 1970 (e.g. year 9999)". For datetime(9999, 12, 31, 23, 59, 59, 999999, tzinfo=utc) the old code gives 253402300800000000 vs 253402300799999999 exact — off by 1µs, not 1s. float64 has 52 bits of mantissa and that value needs ~58 bits, so the ULP there is ~32µs, nowhere near a second. Worth toning down so the comment doesn't send the next reader hunting for a second-scale bug that isn't there.
2. Preserving timestamp()'s naive-datetime behaviour keeps a machine-dependent inconsistency. The comment justifies dt.astimezone() as matching the old "assume local time" behaviour for naive input, which it does. But that behaviour disagrees with how a naive datetime string is handled two functions over — datetime_utils.parse explicitly assumes UTC when there's no offset, matching qdrant core. So on this machine (IST, UTC+5:30):
to_order_value("2024-06-15 12:30:45") # 1718454645000000 (UTC)
to_order_value(datetime(2024, 6, 15, 12, 30, 45)) # 1718434845000000 (local)Same wall-clock time, 19800000000µs apart, and the gap changes with the machine's timezone — so order_by results depend on where the client runs. The aware-UTC object agrees with the string, so it's specifically the naive-object path that diverges.
That's pre-existing and arguably out of scope, but this PR is rewriting exactly this function and pinning the behaviour in a comment as intentional, so it seems like the moment to decide. Making it dt.replace(tzinfo=timezone.utc) would line the object path up with the string path and with core. Happy to file it separately if you'd rather keep this PR purely about precision.
- The year-9999 error is up to ~32us (float64 ULP at that magnitude), not a full second as the comment claimed - verified empirically and corrected per @shashvat-singham's review. - Check dt.utcoffset() is None instead of dt.tzinfo is None to detect naive datetimes: a tzinfo subclass can be attached while still reporting no offset, which the old check would misclassify as aware and crash on `dt - _EPOCH` (TypeError: can't subtract offset-naive and offset-aware datetimes). Per @coderabbitai's review. - Added a regression test for the tzinfo-subclass case, confirmed it fails against the old check and passes with the fix. The naive-datetime-as-local-vs-UTC inconsistency raised in the same review is intentionally left out of this PR - qdrant#1352 already fixes it independently to avoid the two PRs conflicting.
|
Thanks both — pushed an update addressing this.
@coderabbitai: fixed — now checking |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline qdrant_client/local/order_by.py --items all --match 'datetime_to_microseconds'
ast-grep outline tests/test_order_by.py --items all --match 'datetime_to_microseconds|tzinfo|offset'
printf '\n--- implementation references ---\n'
rg -n -C 10 'def datetime_to_microseconds\b|utcoffset\(\)|tzinfo' qdrant_client/local/order_by.py
printf '\n--- regression test references ---\n'
rg -n -C 12 'datetime_to_microseconds\b|utcoffset\(\)|tzinfo|offset-naive' tests/test_order_by.pyLength of output: 6581
The regression test uses an attached 🐇 ✅ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
2 participants
Bug
datetime_to_microsecondscomputesint(dt.timestamp() * 1_000_000).timestamp()returns a float64, and multiplying it by 1e6 before truncating amplifies the rounding error enough to be off by one microsecond for a large fraction of timestamps:In random testing across a range of years, ~35% of timestamps were off by one microsecond. Dates far enough from 1970 (e.g. year 9999) are off by a full second, because the whole-seconds part alone exceeds what float64 can represent precisely alongside the fractional part.
Why it matters
This value feeds ORDER BY comparisons in local mode (
Filterrange/datetime conditions,scrollordering). An off-by-one microsecond can flip a boundary comparison, causing local mode to disagree with what the real Qdrant server returns for the same query - a local/remote congruence bug.Fix
Route through datetime subtraction instead of
timestamp(): subtracting two aware datetimes is exact integer arithmetic internally, no floats involved. A naive input is first attached to the system's local timezone viaastimezone()- the same "assume local time" behaviortimestamp()has for naive input - which itself returns adatetime, not a lossy float, so precision holds all the way through.Testing
Added
tests/test_order_by.py, cross-checking against an independent exact-arithmetic reference implementation across a wide range of years/microseconds, including the year-9999 case. Verified the tests fail against the original implementation and against a first fix attempt that only replaced the multiplication but still usedtimestamp()for the whole-seconds part (that one still got year 9999 wrong by a full second).Opened against
devsince that's where the related recent local-mode fixes (#1259 etc.) landed.