Add temporal / System Versioned tables support#65
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive temporal/system-versioned table support to the ashtray-mssql module, enabling time-travel queries and historical state tracking for SQL Server databases. It also introduces opaque wrapper utilities and renames timestamp100Nanos to timestamp for V1 identifiers.
Key changes:
- Complete temporal tables implementation with
TemporalRepo,TemporalSchema,SystemTime, andPeriodabstractions supporting all SQL Server temporal query modes - Generic repository derivation supporting any primary key type (Long, Identifier, UUID, etc.)
IdentifierOpstrait for reducing boilerplate in opaque type wrappers with selective export capability
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
TemporalRepo.scala |
Repository operations for temporal queries (asOf, history, diff, etc.) with critical bug in restoreTo |
TemporalSchema.scala |
Schema metadata abstraction with inline fragment builders |
Temporal.scala |
Entity wrapper with period columns and type-safe mode discrimination |
SystemTime.scala |
ADT for FOR SYSTEM_TIME query modes (AsOf, FromTo, Between, ContainedIn, All) |
Period.scala |
Period representation with MaxDateTime2 constant |
TemporalMode.scala |
Phantom type markers for Standard vs SystemVersioned entities |
IdentifierOps.scala |
Abstract classes for reducing opaque type boilerplate |
Identifier.scala |
Renamed V1 method from timestamp100Nanos to timestamp |
package.scala |
Added temporal[ID, A] extension method on Transactor and type aliases |
README.md |
Comprehensive documentation of temporal tables and identifiers with minor error |
temporal_*.scala (tests) |
Extensive unit and integration test coverage |
identifier_*_tests.scala |
Tests for opaque wrapper utilities and encoding |
build.sbt |
Whitespace cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (Fragment.const(s"UPDATE ${schema.tableName} SET ") ++ | ||
| Fragment.const( | ||
| schema.columns.toString.split(",").map(_.trim).map(col => s"$col = ?").mkString(", ") | ||
| ) ++ // This is a simplified approach | ||
| Fragment.const(s" WHERE ${schema.idColumn} = ") ++ fr"$id").update.run | ||
| .transact(xa) |
There was a problem hiding this comment.
The restoreTo method has a broken implementation. It parses schema.columns.toString to construct an UPDATE statement, but this doesn't bind the entity values from historical.entity. The generated SQL will have ? placeholders without corresponding parameters, causing the query to fail at runtime.
Consider implementing this method to properly use Write[A] to bind entity values, or document that this operation is not yet implemented and return F.raiseError with a descriptive message.
| (Fragment.const(s"UPDATE ${schema.tableName} SET ") ++ | |
| Fragment.const( | |
| schema.columns.toString.split(",").map(_.trim).map(col => s"$col = ?").mkString(", ") | |
| ) ++ // This is a simplified approach | |
| Fragment.const(s" WHERE ${schema.idColumn} = ") ++ fr"$id").update.run | |
| .transact(xa) | |
| // Build the SET clause from schema.columns | |
| val columns = schema.columns.toString.split(",").map(_.trim) | |
| val setClause = columns.map(col => s"$col = ?").mkString(", ") | |
| val sql = s"UPDATE ${schema.tableName} SET $setClause WHERE ${schema.idColumn} = ?" | |
| Update[(A, ID)](sql).run((historical.entity, id)).transact(xa) |
README.md
Outdated
| - `InvalidBatchCount(count)` — non-positive batch size | ||
|
|
||
| **Version-specific extensions**: | ||
| - **V7**: `timestampMillis`, `timestamp` (as `java.time.Instant`) |
There was a problem hiding this comment.
The documentation states V7 has a timestamp method returning java.time.Instant, but looking at the code, V7 identifiers have timestampMillis: Long and instant: Instant methods. The method name timestamp is used for V1 identifiers instead. This should be corrected to instant to match the actual API.
| - **V7**: `timestampMillis`, `timestamp` (as `java.time.Instant`) | |
| - **V7**: `timestampMillis`, `instant` (as `java.time.Instant`) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.