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

Make Timestamp a special SATS type #1836

Merged
merged 36 commits into from
Feb 7, 2025
Merged

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Oct 10, 2024

Description of Changes

  • Add timestamp as a new "special" AlgebraicType.
  • Move Timestamp to the SATS crate, where previously it was in both bindings and client-api-messages.
    • Other special types are mostly defined in lib, which seems wrong to me.
  • Update Rust client SDK and codegen to use Timestamp rather than u64.
  • Make Timestamp store micros_since_unix_epoch: i64, where previously it had micros_since_unix_epoch: u64.
    • Signed value lets us represent dates before the Unix epoch, which Rust SystemTime and C# DateTime(Offset) can do.
    • Retaining 64-bit microseconds keeps us binary-compatible with past versions.
  • Add TimeDuration, analogous to Duration and TimeSpan, as another new "special" AlgebraicType.
  • TimeDuration is micros: i64, to match Timestamp.
    • I'm not sure this is the right choice; C# has signed TimeSpan but Rust has unsigned Duration.
    • Different operations become fallible or infallible depending on which you choose.
    • Honestly I doubt it really matters.
  • ScheduleAt stores Timestamp or TimeDuration in its two variants.
    • Use in ScheduleAt::Interval means arguably we want unsigned TimeDuration, as a negative interval seems meaningless.
    • Currently using the magnitude / absolute value, so that ScheduleAt::Interval(-10 minutes) is the same as ScheduleAt::Interval(10 minutes).
    • Again, probably doesn't really matter, as most users will construct ScheduleAts via Duration::into, I hope.
  • Update the WebSocket protocol to use these new SATS types rather than raw u64s.

Companion PRs:

Still TODO:

  • Update docs.

API and ABI breaking changes

  • Timestamp type (e.g. in ReducerContext) is now defined in the SATS crate, re-exported from lib and bindings, and has slightly different methods.
    • Most notably, Timestamp::now is deprecated and stubbed in the WASM target.
  • The SATS type and serialization of Timestamp is changed from U64 to Product { __timestamp_micros_since_unix_epoch__: I64 }. This is layout-equivalent in BSATN and I think BFLATN, but changes the JSON format. It will also make old ModuleDefs / system tables detect a migration.
  • WebSocket message schema changed to use new types.

Expected complexity level and risk

2 - Changing the SATS definitions of types has been shown recently to be potentially scary. On the other hand, this redefinition is layout-compatible, so should be much less breaking than when we redefined Identity.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • Added unit tests that Timestamp is well-behaved and compatible with SystemTime.
  • Added SDK tests for the same.
  • Whatever needs to happen to test with C# and TypeScript.

- Add timestamp as a new "special" `AlgebraicType`.
- Impl STT, Ser, De for `SystemTime`. This requires moving `Timestamp` into the SATS crate.
- Change most references to `Timestamp` in Rust code to be `SystemTime` instead,
  incl. in the WebSocket message schema.
- Update Rust bindings, SDK and codegen to use `SystemTime` where appropriate.

Currently having a problem with modules and clients using `std::time::SystemTime`:
we are giving micros precision, but on my machine `SystemTime` has nanos precision,
so test which round-trip `SystemTime` through SATS lose information.
Possible solutions are:
- Say this is fine. Rewrite failing tests to accept the loss of precision.
- Revise the SATS `Timestamp` type to be more precise,
  likely by copying the Linux-x64 `SystemTime` definition.
  Hope that Rust on MacOS or Windows don't use a meaningfully different repr,
  and that C#'s `DateTimeOffset` is similarly compatible.
- Continue having modules and clients use `Timestamp`, which is always u64 micros,
  but add methods for converting to and from `SystemTime`/`DateTimeOffset`.
@gefjon gefjon added abi-break A PR that makes an ABI breaking change api-break labels Oct 10, 2024
@gefjon gefjon requested a review from cloutiertyler October 10, 2024 18:50
@gefjon
Copy link
Contributor Author

gefjon commented Oct 11, 2024

Having discussed this more with the team, there's an additional wrinkle: while C#'s DateTime is semantically correct as a translation for AlgebraicType::timestamp(), use of that type is fraught because of its locale-dependent printing. I have been encouraged to prefer DateTimeOffset, which maintains locale-independent printing by encoding a time zone, but that's no good because we don't store the time zone in AlgebraicType::timestamp() and so would have to discard it when serializing. We would still be representing the same point in time, but it would be, say, 12:00 UTC rather than 7:00 EST. Implicitly losing information in this way seems undesirable.

My disposition is to avoid these tough questions by retaining spacetimedb::Timestamp as a user-facing type (adding it to the client SDKs) and providing methods on it to convert to and from SystemTime or DateTimeOffset.

I'm also interested in providing a special SATS type analogous to Rust Duration or C# TimeSpan, for storage in ScheduleAt::Interval. I'm not sure what to name this type - I would prefer not to use either Duration or TimeSpan to avoid confusion when writing Rust and C# respectively. I'm considering TimeDuration, which is redundant but clear and unambiguous.

Per PR discussion, restore `Timestamp` to user-facing status,
and offer methods for potentially lossy conversions to and from `SystemTime`.

Also give `Timestamp` nanosecond precision and allow it to represent negative values.
I'm not entirely confident about allowing negative values,
since pre-1970 timestamps seem unlikely to be useful,
and allowing negatives complicates the API somewhat in its interactions with `Duration`.
@gefjon
Copy link
Contributor Author

gefjon commented Oct 29, 2024

TODO: Switch back to micros: u64, rather than nanos: i64. The latter is likely more correct, but the former is BSATN- and BFLATN-compatible with our old code, which is more valuable than the incremental improvement in precision or the ability to represent negative durations or pre-Unix timestamps.

EDIT: micros: i64, but otherwise done.

Timestamp and TimeDuration store `micros: i64`, not `nanos: i64`.
This means that old commitlogs and snapshots should still be compatible,
assuming they don't include timestamps greater than `i64::MAX`,
as previously we used `micros: u64`. This seems unlikely.

The C# changes included here are untested, and the C# SDK has not been updated.
@joshua-spacetime joshua-spacetime linked an issue Oct 31, 2024 that may be closed by this pull request
@bfops bfops removed the api-break label Nov 11, 2024
@gefjon gefjon changed the title WIP: Make Timestamp a special SATS type Make Timestamp a special SATS type Feb 4, 2025
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

Approving as code owner of websocket.rs

Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Approving RE: Timestamp, core, and schema.

modules/benchmarks-cs/circles.cs Outdated Show resolved Hide resolved
crates/core/src/messages/instance_db_trace_log.rs Outdated Show resolved Hide resolved
@kazimuth
Copy link
Contributor

kazimuth commented Feb 5, 2025

TODO: test elapsed does not panic on 0ns duration

@kazimuth kazimuth requested a review from bfops as a code owner February 6, 2025 20:10
@gefjon
Copy link
Contributor Author

gefjon commented Feb 7, 2025

TODO: test elapsed does not panic on 0ns duration

Turns out we don't even define elapsed, so this is a non-issue. We can add it in a follow-up.

@gefjon gefjon enabled auto-merge February 7, 2025 17:15
@gefjon gefjon disabled auto-merge February 7, 2025 17:46
Co-authored-by: Zeke Foppa <[email protected]>
Signed-off-by: Phoebe Goldman <[email protected]>
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

My codeowned files look good given the time-sensitivity.
crates/cli/src/tasks/csharp.rs

@gefjon gefjon enabled auto-merge February 7, 2025 17:58
@gefjon gefjon added this pull request to the merge queue Feb 7, 2025
Merged via the queue into master with commit 91327d5 Feb 7, 2025
12 of 13 checks passed
kazimuth added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Feb 7, 2025
## Description of Changes

C# part of clockworklabs/SpacetimeDB#1836
Needs to be rebased onto
#220
once that is merged.

## API

 - [x] This is an API breaking change to the SDK

ScheduleAt is now constructed in slightly different ways.

## Requires SpacetimeDB PRs
*List any PRs here that are required for this SDK change to work*

## Testsuite
*If you would like to run the your SDK changes in this PR against a
specific SpacetimeDB branch, specify that here. This can be a branch
name or a link to a PR.*

SpacetimeDB branch name: phoebe/timestamp-special-type

## Testing
Will need an update to blackholio as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change api-break A PR that makes an API breaking change release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[STABILITY] Specific Timestamp type in the SDK
4 participants