Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

fix: remove floating point rounding error in Timestamp.fromMillis()#1464

Merged
thebrianchen merged 2 commits intomasterfrom
bc/timestamp
Apr 7, 2021
Merged

fix: remove floating point rounding error in Timestamp.fromMillis()#1464
thebrianchen merged 2 commits intomasterfrom
bc/timestamp

Conversation

@thebrianchen
Copy link
Copy Markdown

Fixes #1463.

By multiplying MS_TO_NANOS separately, we avoid the floating point precision rounding errors.

@thebrianchen thebrianchen requested review from a team April 6, 2021 19:26
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label Apr 6, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 6, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 6, 2021

Codecov Report

Merging #1464 (d6267ef) into master (42ade78) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1464      +/-   ##
==========================================
- Coverage   98.21%   98.20%   -0.02%     
==========================================
  Files          32       32              
  Lines       19567    19569       +2     
  Branches     1373     1285      -88     
==========================================
  Hits        19217    19217              
- Misses        346      348       +2     
  Partials        4        4              
Impacted Files Coverage Δ
dev/src/timestamp.ts 100.00% <100.00%> (ø)
dev/src/v1beta1/firestore_client.ts 97.62% <0.00%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42ade78...d6267ef. Read the comment docs.

Comment thread dev/src/timestamp.ts Outdated
static fromMillis(milliseconds: number): Timestamp {
const seconds = Math.floor(milliseconds / 1000);
const nanos = (milliseconds - seconds * 1000) * MS_TO_NANOS;
const nanos = Math.round(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to round down here so that nanos is always less than "one second in nanos".

@schmidt-sebastian schmidt-sebastian removed their assignment Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: firestore Issues related to the googleapis/nodejs-firestore API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timestamp.fromMillis can't handle nanoseconds.

2 participants