fix: repair the benchmarks build and a flaky identity-hash key - #23
Merged
Conversation
Two pre-existing problems that made verification unreliable, both independent of the query work that follows. TychoDB.Benchmarks has not compiled since 123c79f, which added an IJsonSerializer parameter to SortBuilder.Build so property expressions could render against the serializer's member names. One call site in Diagnostics.cs was never updated, so `dotnet build TychoDB.sln` failed outright and nobody could run the benchmarks backing our perf claims. TychoDb_QueryUsingContains_ShouldBeSuccessful keyed 1000 objects by GetHashCode() on a type that does not override it — the runtime identity hash, not a value hash. Instances collide, INSERT OR REPLACE overwrites, and the count assertion fails at random. Measured over 2000 trials of building 1000 TestClassA instances: 16 trials collided (0.80%), with as few as 998 distinct keys. Observed twice as a real suite failure across roughly 20 full runs. Keyed by IntProperty instead, which is already distinct over Range(100, 1000). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes two independent issues that made verification unreliable: a broken benchmarks build due to an outdated SortBuilder.Build call site, and a flaky unit test caused by using runtime identity hashes as keys (leading to occasional key collisions and overwritten rows).
Changes:
- Update
TychoDB.Benchmarksdiagnostics sort-query generation to pass anIJsonSerializerintoSortBuilder.Build. - Make
TychoDb_QueryUsingContains_ShouldBeSuccessfuldeterministic by keying writes onIntPropertyinstead ofObject.GetHashCode().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| TychoDB.UnitTests/TychoDbTests.cs | Replaces identity-hash-based keys with a deterministic value key to eliminate flaky collisions; adds explanatory comment. |
| TychoDB.Benchmarks/Diagnostics.cs | Fixes build by updating SortBuilder.Build invocation to include the required serializer parameter. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two pre-existing problems that made verification unreliable. Both independent of the query work in the rest of this stack, split out first because the benchmarks fix is what makes the solution build at all.
TychoDB.Benchmarkshas not compiled since 123c79fThat commit added an
IJsonSerializerparameter toSortBuilder.Buildso property expressions render against the serializer's member names. One call site inDiagnostics.cswas never updated, sodotnet build TychoDB.slnfailed outright — nobody could run the benchmarks that back our performance claims.A test keyed 1000 objects by their runtime identity hash
TychoDb_QueryUsingContains_ShouldBeSuccessfulusedx => x.GetHashCode().ToString()on a type that doesn't overrideGetHashCode. That's the identity hash, not a value hash: instances collide,INSERT OR REPLACEoverwrites, andCount().ShouldBe(1000)fails.Measured over 2000 trials of building 1000
TestClassAinstances: 16 collided (0.80%), with as few as 998 distinct keys. Observed twice as a real suite failure across roughly 20 full runs while working on the rest of this stack.Now keyed by
IntProperty, already distinct overRange(100, 1000)— deterministic by construction rather than 0.8%-per-run.The other four
GetHashCode()uses in that file are left alone: they write a single object and read it back by the same instance's hash, so there's only ever one key and no collision is possible.Verification
dotnet build TychoDB.sln -c Releaseclean; 227 tests pass.🤖 Generated with Claude Code