-
Notifications
You must be signed in to change notification settings - Fork 4
Add Versionstamp and Subspace support to Swift bindings #16
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
base: main
Are you sure you want to change the base?
Add Versionstamp and Subspace support to Swift bindings #16
Conversation
This commit adds two major features to the Swift bindings: ## Versionstamp Support - Implement 12-byte Versionstamp structure (10-byte transaction version + 2-byte user version) - Add incomplete versionstamp support for transaction-time assignment - Implement Tuple integration with versionstamp encoding (type code 0x33) - Add packWithVersionstamp() for atomic operations ## Subspace Implementation - Implement Subspace for key namespace management with tuple encoding - Add range() method using prefix + [0x00] / prefix + [0xFF] pattern - Implement strinc() algorithm for raw binary prefix support - Add prefixRange() method for complete prefix coverage - Define SubspaceError for proper error handling ## Testing - Add VersionstampTests with 15 test cases - Add StringIncrementTests with 14 test cases for strinc() algorithm - Add SubspaceTests with 22 test cases covering range() and prefixRange() - Verify cross-language compatibility with official bindings All implementations follow the canonical behavior of official Java, Python, Go, and C++ bindings.
This commit completes the Versionstamp implementation by adding decode support and comprehensive roundtrip tests. ## Tuple.decode() Integration - Add versionstamp case (0x33) to Tuple.decode() switch - Enable automatic Versionstamp decoding in tuples - Allows reading versionstamped keys from database ## Roundtrip Tests - Add 5 roundtrip tests (encode → decode) - Complete versionstamp roundtrip - Incomplete versionstamp roundtrip - Mixed tuple with multiple types - Multiple versionstamps in one tuple - Error handling for insufficient bytes ## Test Fixes - Fix withUnsafeBytes crash by ensuring exact 4-byte array - Add size validation before unsafe memory access - Fix range test expectations (prefix vs prefix + [0x00]) ## Code Cleanup - Remove dead code for API < 520 (no longer supported) - Simplify to single code path using 4-byte offsets - Update documentation to reflect API 520+ requirement All 150 tests now pass successfully.
Sources/FoundationDB/Subspace.swift
Outdated
| let tupleBytes = Array(key.dropFirst(prefix.count)) | ||
| let elements = try Tuple.decode(from: tupleBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite inefficient: You're allocating a new array and then copying all but one element from the key array. It seems like the various decode methods might be better implemented as being generic over a Collection<UInt8>. That would allow you to decode a slice without going via an intermediate array.
Sources/FoundationDB/Subspace.swift
Outdated
| extension Subspace: Equatable { | ||
| public static func == (lhs: Subspace, rhs: Subspace) -> Bool { | ||
| return lhs.prefix == rhs.prefix | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Hashable | ||
|
|
||
| extension Subspace: Hashable { | ||
| public func hash(into hasher: inout Hasher) { | ||
| hasher.combine(prefix) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the compiler not synthesise these conformances for you?
Sources/FoundationDB/Subspace.swift
Outdated
| // MARK: - SubspaceError | ||
|
|
||
| /// Errors that can occur in Subspace operations | ||
| public enum SubspaceError: Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enums generally make for bad error types as you can't add new cases without breaking API.
The most common model is having an error code and an error message wrapped up in a struct. It's not uncommon to then attach a few other things too (like an underlying error if applicable). SwiftNIO's FileSystemError is a good example of this (see here).
Sources/FoundationDB/Subspace.swift
Outdated
| /// ``` | ||
| /// | ||
| /// - SeeAlso: `Subspace.prefixRange()` for usage with Subspace | ||
| public func strinc() throws -> FDB.Bytes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to call out again that this is another reason we're pretty strongly opposed to using typealiases in public APIs: any extension on the typealias pollutes the aliased type.
Sources/FoundationDB/Subspace.swift
Outdated
| } | ||
|
|
||
| // Check if result is empty (input was empty or all 0xFF) | ||
| guard !result.isEmpty else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: guard !X is less readable than if X
| return elements.reduce(0) { count, element in | ||
| if let vs = element as? Versionstamp, !vs.isComplete { | ||
| return count + 1 | ||
| } | ||
| return count | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a count(where:) method you can use for this:
return elements.count {
if let vs = element as? Versionstamp {
return !vs.isComplete
}
return false
}| guard incompleteCount == 1 else { | ||
| throw TupleError.invalidEncoding | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guard is great for early returns. The else branch of the guard here is no earlier than the guarded path; if would be more idiomatic and easier to read here:
if incompleteCount != 1 {
throw TupleError.invalidEncoding
}| offset += 1 | ||
|
|
||
| switch typeCode { | ||
| case TupleTypeCode.versionstamp.rawValue: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be more idiomatic to turn your typeCode into a typed TupleTypeCode so that you can switch over it more naturally.
| var bytes = transactionVersion ?? Self.incompletePlaceholder | ||
|
|
||
| // User version is stored as big-endian | ||
| bytes.append(contentsOf: withUnsafeBytes(of: userVersion.bigEndian) { Array($0) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another avoidable array allocation.
| public func hash(into hasher: inout Hasher) { | ||
| hasher.combine(transactionVersion) | ||
| hasher.combine(userVersion) | ||
| } | ||
|
|
||
| public static func == (lhs: Versionstamp, rhs: Versionstamp) -> Bool { | ||
| return lhs.transactionVersion == rhs.transactionVersion && | ||
| lhs.userVersion == rhs.userVersion | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason the compiler can't synthesise these?
Sources/FoundationDB/Subspace.swift
Outdated
|
|
||
| /// Create a subspace with a string prefix | ||
| /// - Parameter rootPrefix: The string prefix (will be encoded as a Tuple) | ||
| public init(rootPrefix: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the need to privilege strings like this. In practice, I would expect that the root of a subspace would come from the Directory Layer, which we do not yet have for Swift, but should.
| /// - Returns: The encoded key with prefix | ||
| /// | ||
| /// The returned key will have the format: `[prefix][encoded tuple]` | ||
| public func pack(_ tuple: Tuple) -> FDB.Bytes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong preference between pack / unpack and encode / decode, but I do not see any reason for Subspace and Tuple to differ in their choice. Perhaps others can comment on which feels more natural in this binding.
| /// let activeUsers = users.subspace("active") // prefix = users + "active" | ||
| /// let userById = activeUsers.subspace(12345) // prefix = users + "active" + 12345 | ||
| /// ``` | ||
| public func subspace(_ elements: any TupleElement...) -> Subspace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other bindings also overload subscript to do this. For example, Python adds a __getitem__. Although Rust's Subspace does not implement Index, so it is not universal. But I have found it convenient.
Remove manual implementations of Equatable and Hashable in Versionstamp and Subspace. The compiler can synthesize these conformances automatically since all stored properties already conform to these protocols. This improves code maintainability and follows Swift best practices. Addresses: glbrntt review comment FoundationDB#2
- Use count(where:) instead of reduce for counting incomplete versionstamps This is more idiomatic Swift and clearly expresses intent - Replace guard with if for incompleteCount validation Guard is for early returns; if is more appropriate here - Optimize withUnsafeBytes to eliminate unnecessary allocation Append directly instead of creating intermediate Array Addresses: glbrntt review comments FoundationDB#6, FoundationDB#7, FoundationDB#10
Move strinc() from FDB.Bytes extension to FDB static method. Since FDB.Bytes is a typealias for [UInt8], extending it would pollute all Array<UInt8> types with the strinc() method in the public API. The new API matches other language bindings more closely: - Go: fdb.Strinc() - Python: fdb.strinc() - Java: ByteArrayUtil.strinc() Usage: try FDB.strinc(bytes) instead of try bytes.strinc() Addresses: glbrntt review comment FoundationDB#4
Convert SubspaceError from enum to struct-based error with error codes. This design follows SwiftNIO's FileSystemError pattern and provides better extensibility for future error cases. The struct-based approach will make it easier to add new error types when the Directory Layer is introduced in the next PR. Also replace guard with if in strinc error check for better readability, as guard is intended for early returns. Addresses: glbrntt review comments FoundationDB#3, FoundationDB#5
1. Use TupleTypeCode enum in switch statements for type safety
Convert raw UInt8 type code to TupleTypeCode enum before switching.
This provides compile-time safety and makes the code more maintainable.
2. Rename encode/decode to pack/unpack for cross-language consistency
All official FoundationDB bindings use pack/unpack terminology:
- Python: tuple.pack() / tuple.unpack()
- Java: Tuple.pack() / Tuple.fromBytes()
- Go: Tuple.Pack() / Tuple.Unpack()
Using pack/unpack:
- Matches established FDB terminology ("tuple packing")
- Avoids confusion with Swift's Codable encode/decode
- Improves searchability and documentation consistency
API changes:
- Tuple.encode() → Tuple.pack()
- Tuple.decode(from:) → Tuple.unpack(from:)
- Subspace methods already used pack/unpack (no change)
Addresses: glbrntt review comment FoundationDB#9, MMcM review comment FoundationDB#2
Update all tests to reflect API changes: - Use Tuple.pack() / Tuple.unpack() instead of encode/decode - Use Tuple().pack() for Subspace prefix initialization - Update SubspaceError checks to use struct-based error with .code - Update StackTester to use new API All 150 tests pass successfully.
pack/unpack Naming DecisionThank you for raising this important design question. After careful consideration, I've decided to standardize on Current State AnalysisThe Swift bindings initially had inconsistent naming:
This inconsistency needed resolution. Cross-Language ConsistencyAll official FoundationDB bindings use pack/unpack: Python: Java: Go: C++ (via bindings documentation): Uses "pack" terminology Why `pack/unpack` Instead of `encode/decode`1. Established FDB Terminology
2. Avoids Confusion with Swift's CodableSwift developers strongly associate `encode/decode` with the `Codable` protocol: Using `pack/unpack` clearly signals this is FDB-specific tuple encoding, not general Swift serialization. 3. Semantic Clarity
Tuple packing is special—it preserves lexicographic ordering, which is critical for range queries. 4. Internal vs Public APIThe `TupleElement` protocol still uses `encodeTuple()`/`decodeTuple()` internally (for implementation), while the public API uses `pack()`/`unpack()` (for users). This separation is appropriate: // Internal protocol - implementation detail ImplementationI've updated the code to use `pack/unpack` consistently:
All 150 tests pass with the new API. Benefits
This decision prioritizes FoundationDB ecosystem consistency over Swift-specific conventions, which is appropriate for a database client library where cross-language compatibility is valuable. |
|
@glbrntt @MMcM Thank you both very much for your thorough and insightful code reviews! Your feedback has significantly improved the code quality, Swift idiomaticity, and API design of this PR. Changes Implemented@glbrntt's review comments:
@MMcM's review comments:
All 150 tests pass successfully after these changes. Ready for Re-reviewI've committed these changes in separate, focused commits for easier review:
The code is now more idiomatic Swift while maintaining strong cross-language compatibility. Note: I'm not a native English speaker and am using AI assistance to help communicate my intent clearly. Please let me know if anything needs clarification. Thank you again for your time and expertise! I look forward to your feedback. |
Summary
This PR adds two major missing features to the Swift bindings:
These features bring the Swift bindings to parity with Python, Go, and Java bindings, enabling the development of Record Layer and other advanced FoundationDB applications in Swift.
Motivation
The Swift bindings currently lack essential features available in other language bindings, making it difficult to:
This PR addresses these gaps by implementing the missing features following the canonical behavior of official bindings.
Versionstamp Implementation
Core Features
Versionstampstruct: 12-byte value (10-byte transaction version + 2-byte user version)packWithVersionstamp(): Automatic offset calculation for atomic operationsUsage Example
Subspace Implementation
Core Features
Subspacestruct: Key namespace management with tuple encodingrange()method: Returns(prefix + [0x00], prefix + [0xFF])for tuple datastrinc()algorithm: String increment for raw binary prefixesprefixRange()method: Complete prefix coverage using strincUsage Example
Testing
Comprehensive test suite with 150 tests (all passing):
Versionstamp Tests (20)
Subspace Tests (22)
range()andprefixRange())String Increment Tests (14)
Integration
Compatibility
Cross-Language Consistency
This implementation follows the canonical behavior of official bindings:
ByteArrayUtil.strinc(),Versionstamp,Subspacefdb.strinc(),fdb.tuple.Versionstamp, tuple packingfdb.Strinc(),fdb.IncompleteVersionstamp(),Subspace.FDBRangeKeys()API Version
Files Changed
New Files
Sources/FoundationDB/Versionstamp.swift(196 lines)Sources/FoundationDB/Tuple+Versionstamp.swift(205 lines)packWithVersionstamp()methodSources/FoundationDB/Subspace.swift(424 lines)range()andprefixRange()methodsstrinc()algorithmTests/FoundationDBTests/VersionstampTests.swift(416 lines)Tests/FoundationDBTests/SubspaceTests.swift(312 lines)Tests/FoundationDBTests/StringIncrementTests.swift(194 lines)Modified Files
Sources/FoundationDB/Tuple.swift(+3 lines)Tuple.decode()switchStatistics
Breaking Changes
None. This PR is purely additive and maintains full backward compatibility with existing code.
Future Work
This PR lays the groundwork for:
Checklist
References
bindings/python/fdb/tuple.pybindings/go/src/fdb/tuple/tuple.gobindings/java/src/main/com/apple/foundationdb/tuple/Versionstamp.java