fix: normalize TraceRun events to ascending sequence at construction - #56
Merged
Merged
Conversation
Mirrors DProvenanceKit PR #73. TraceEvent's own docstring declares sequence the authoritative causal order, but TraceRun kept whatever list order the caller assembled: the in-memory temporal query evaluator (after/before) used list order — diverging from the SQL backend, which orders by MIN(sequence), and from TRACE_SPEC_v1 — and the diff engine manufactured spurious added/removed pairs for hand-assembled out-of-order runs. align() already sorted internally, so alignment was correct but inconsistent with the other two surfaces. TraceRun now sorts events by sequence in __post_init__ (Python's sort is stable, so duplicate sequences keep the caller's relative order). Store load paths already delivered sorted lists and are unaffected. New test module pins the sort, the stable tie-break, verbatim preservation of sorted input, SQL-consistent temporal queries on unsorted input, and diff assembly-order invariance.
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.
Mirrors DProvenanceKit PR #73 (merged), keeping the two SDKs' ordering contracts identical.
Problem
TraceEvent's docstring declaressequencethe authoritative causal order, butTraceRunkept the caller's list order, and the three analysis surfaces disagreed about what order means:after/before, query.pyAfterNode/BeforeNode) used list order — diverging from the SQL backend, which orders byMIN(sequence)(TRACE_SPEC_v1 §temporal operators), so the same query on the same hand-built run answered differently depending on the store;Store load paths always delivered sequence-ordered lists, so this bit only hand-constructed runs (e.g. events decoded from a consumer's own JSON).
Fix
TraceRun.__post_init__normalizeseventsto ascendingsequence(stable sort — duplicate sequences keep assembly order), mirroring the Swift initializer. All three surfaces now share one ordering basis.Verification
tests/test_tracerun_normalization.py(5 tests, portsTraceRunNormalizationTests): sort, stable tie-break, verbatim sorted input, SQL-consistent temporal query on unsorted input, diff invariance.Follow-up (both repos)
Conformance vectors carry no explicit
sequencefield — both harnesses assign sequence = index, so vectors are structurally blind to ordering bugs like this one. A vector class with explicit out-of-order sequences needs a coordinated spec addition in both repos; tracked for a separate PR.