Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add missing nano part when encoding timestamp to document #1576

Merged
merged 4 commits into from
Sep 2, 2024

Conversation

ghostbuster91
Copy link
Contributor

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@@ -101,7 +101,7 @@ class DocumentEncoderSchemaVisitor(
.getOrElse(TimestampFormat.EPOCH_SECONDS) match {
case DATE_TIME => ts => DString(ts.format(DATE_TIME))
case HTTP_DATE => ts => DString(ts.format(HTTP_DATE))
case EPOCH_SECONDS => ts => DNumber(BigDecimal(ts.epochSecond))
case EPOCH_SECONDS => ts => DNumber(BigDecimal(s"${ts.epochSecond}.${ts.nano}"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought: I am not really a fan of constructing bigDecimal from string in here but I couldn't think of anything better.

Copy link
Contributor

@Baccata Baccata Sep 1, 2024

Choose a reason for hiding this comment

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

BigDecimal(ts.epochSecond + 10e-9 * ts.nano) ?

Copy link
Contributor Author

@ghostbuster91 ghostbuster91 Sep 2, 2024

Choose a reason for hiding this comment

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

I should've made myself clearer. I thought about this approach, but I didn't want to use because of potential precision loss issue.

Consider following:

val now = Instant.now()
val epochSecond: Long = now.getEpochSecond
val nano: Int = 999_999_999 // almost 1 second

val result: Double = epochSecond + 1e-9 * nano
println(s"Result with potential precision loss: ${BigDecimal(result)}")

val accurateBigDecimal =
  BigDecimal(epochSecond) + (BigDecimal(nano) * BigDecimal(10).pow(-9))

println(s"Accurate result : ${accurateBigDecimal}")

Which prints:

Result with potential precision loss: 1725278847
Accurate result : 1725278846.999999999

This is still probably a better idea than going through string parsing :)

@Baccata Baccata merged commit 3608c82 into disneystreaming:series/0.18 Sep 2, 2024
11 checks passed
@ghostbuster91 ghostbuster91 deleted the fix-missing-nano branch September 2, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants