M2-13: canonical Bar resampling and interval materialization - #96
Conversation
Adds Manager.Build, executing exactly the ActionNormalizeCanonical and ActionDeriveCanonical entries in a Plan, mirroring Sync's own "only these actions" scope split. - normalizeAndPublish: raw -> same-interval canonical, wiring #76's normalizer to publication for the first time. Aborts the whole partition (no partial publish) on any Suspicious/Rejected record; excludes Incomplete records without aborting. - deriveAndPublish + aggregateBars: resamples canonical D1 into canonical W1, per calendar week, re-checking D1 completeness against Coverage independently of whatever range originally produced the Action. Aggregation formula (OHLC/ticks/max-spread/tick-weighted avg-spread) mined from trader-first-try/datamanager/candle_agg.go's aggregateWindow; legacy never actually built W1, so this is the formula applied to a new resampling target, not transplanted code. - oanda.FingerprintPartition: targeted single-partition fingerprint, reused by normalizeAndPublish for Manifest.RawFingerprint. - Manager.readAllBars: Bars-draining convenience wrapper, needed because Bars requires full coverage of its queried range while Coverage does not — deriveAndPublish checks D1 readiness once for the whole month via Coverage, then loads bars per ready week only. - build_corpus_test.go (corpus build tag, operator-run, excluded from CI): H4/D1 corpus comparison against OANDA-native partitions, and a one-time legacy candle-v2 comparison. A real run against the preserved archive confirmed D1 derivation agrees with native OANDA D1 (disagreements found only at the comparison tool's own single-partition month-boundary limitation) and surfaced a real quality finding: OANDA's native H4 anchors to the FX daily rollover, not UTC-midnight truncation as ADR-012 currently specifies for H4. Recorded as a finding, not silently fixed — revising H4 alignment is a future ADR-012 supersession, out of this issue's scope. Tested via build_test.go/resample_test.go (marketdata 92.3%, oanda 92.1% coverage) plus a real corpus run (constants reverted to empty before commit). make check passes. Documentation: ADR-020 DONE section, marketdata/doc.go, oanda/doc.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCR2bDEEGEsBfyu4hKu9f2
There was a problem hiding this comment.
Pull request overview
This PR introduces a canonical “build” path in marketdata.Manager to materialize canonical datasets from a precomputed Plan, complementing Sync by handling only canonical actions (normalize + derive). It wires raw→canonical publication for the first time and adds D1→W1 resampling to materialize W1 (an interval with no raw provider source), along with documentation and corpus-scale operator tooling.
Changes:
- Add
Manager.Buildorchestration plus supporting publication/caching plumbing for canonical normalization and derivation. - Implement D1→W1 resampling (
deriveAndPublish,aggregateBars) and supporting helpers (weekIsD1Ready,readAllBars). - Add raw single-partition fingerprinting and expand tests/docs (unit tests + build-tagged corpus operator tests + ADR/docs updates).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| marketdata/build.go | Adds Manager.Build, build result types, version constants, and canonical publish helper w/ cache invalidation. |
| marketdata/build_normalize.go | Implements raw→canonical normalization + publication with abort-on-bad-record semantics. |
| marketdata/resample.go | Implements D1→W1 derivation + publication and the aggregation logic. |
| marketdata/query.go | Adds internal readAllBars helper to drain Bars() into a slice for resampling. |
| marketdata/build_test.go | Adds tests covering Build’s normalize/derive paths, skipping behavior, cancellation, and cache invalidation. |
| marketdata/resample_test.go | Adds focused unit tests for aggregateBars and weekIsD1Ready. |
| marketdata/build_corpus_test.go | Adds build-tagged (corpus) operator-run corpus comparison tooling against real archives. |
| marketdata/internal/provider/oanda/writer.go | Adds FingerprintPartition helper for targeted raw partition hashing. |
| marketdata/internal/provider/oanda/writer_test.go | Adds tests validating FingerprintPartition output and error behavior. |
| marketdata/internal/provider/oanda/doc.go | Documents FingerprintPartition and its intended use in canonical build. |
| marketdata/doc.go | Documents Manager.Build, its scope split vs Sync, and normalize/derive semantics. |
| docs/arch/adr-020-historic-data.org | Records the resampling/materialization decisions, formulas, and corpus-run findings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| totalTicks += b.Ticks | ||
| if b.Ticks <= 0 { | ||
| continue | ||
| } | ||
| weight := num.MustParseRate(strconv.FormatInt(b.Ticks, 10)) | ||
| contribution, err := b.AvgSpread.MulRate(weight) | ||
| if err != nil { | ||
| return Bar{}, fmt.Errorf("weight spread: %w", err) | ||
| } | ||
| if !haveWeighted { | ||
| weightedSum = contribution | ||
| haveWeighted = true | ||
| continue | ||
| } | ||
| weightedSum, err = weightedSum.Add(contribution) | ||
| if err != nil { | ||
| return Bar{}, fmt.Errorf("sum weighted spread: %w", err) | ||
| } | ||
| } | ||
| agg.Ticks = totalTicks | ||
|
|
||
| if totalTicks > 0 { | ||
| inv, err := num.MustParseRate("1").DivRate(num.MustParseRate(strconv.FormatInt(totalTicks, 10))) | ||
| if err != nil { | ||
| return Bar{}, fmt.Errorf("compute weight inverse: %w", err) | ||
| } |
There was a problem hiding this comment.
Fixed: aggregateBars now uses num.ParseRate (not MustParseRate) on both the per-bar tick weight and the total-tick inverse, propagating the error instead of risking a panic on a large tick count. Commit 3ea9620.
rustyeddy
left a comment
There was a problem hiding this comment.
The overall Build/Sync capability split and normalization policy look good, but I found four correctness/contract issues that should be addressed before merge. I also agree with Copilot's two existing comments (MustParseRate on runtime values and partial results from readAllBars). Latest CI is green.
| if !m.configured() { | ||
| return BuildResult{}, fmt.Errorf("marketdata: build: %w: manager is not configured", ErrInvalidConfig) | ||
| } | ||
| if m.rawRoot == "" { |
There was a problem hiding this comment.
This unconditional check prevents a W1-only build even though ActionDeriveCanonical reads only canonical D1. It also makes a plan containing only skipped raw actions fail before those actions can be reported in Skipped. That contradicts deriveActionsW1's documented support for a Manager with no RawRoot. Please require RawRoot only when executing ActionNormalizeCanonical (where the dependency is actually used), and add a W1-only/no-raw-root test.
There was a problem hiding this comment.
Fixed: the RawRoot check moved out of Build and into normalizeAndPublish (the only path that actually reads raw data), so a W1-only Plan — or a Plan whose only entries are ActionDownloadRaw/ActionRepairRaw, reported in Skipped — now works with no RawRoot configured. Added TestBuild_DeriveWorksWithNoRawRoot and TestBuild_PlanWithOnlySkippedActionsReportsThemWithoutRawRoot. Commit 3ea9620.
| } | ||
| } | ||
|
|
||
| fingerprint, err := oanda.FingerprintPartition(m.rawRoot, symbol, rawInterval, action.Year, action.Month) |
There was a problem hiding this comment.
The records and their recorded fingerprint come from two separate opens. Sync can atomically replace this raw file after ReadPartitionRecords closes it but before FingerprintPartition reads it, causing canonical bars from revision A to be published with revision B's fingerprint. Please parse and hash the same opened file/snapshot (or otherwise verify a stable snapshot) so Manifest.RawFingerprint necessarily identifies the bytes that produced the bars; add a replacement-between-read-and-hash regression test.
There was a problem hiding this comment.
Fixed: added oanda.ReadPartitionSnapshot, a single os.ReadFile that both fingerprints and parses the identical in-memory bytes, replacing the previous ReadPartitionRecords+FingerprintPartition two-open pattern. normalizeAndPublish now uses it exclusively. Added TestReadPartitionSnapshot_ConsistentUnderConcurrentReplace, which races a WritePartition replace against ReadPartitionSnapshot under -race and asserts every observed snapshot's record count and fingerprint always describe the same file revision. Commit 3ea9620.
| // most months have some not-yet-ready weeks, and a single whole- | ||
| // month Bars call would fail on the first one of those rather than | ||
| // let the ready weeks publish. | ||
| d1Query := BarQuery{Instrument: action.Instrument, Interval: D1, Range: monthSpan} |
There was a problem hiding this comment.
d1Cov covers only [monthStart, monthEnd), but the last W1 span whose start belongs to this month commonly extends into the next month. weekIsD1Ready therefore cannot see a missing/stale/invalid adjacent D1 partition. If that partition is absent, this week is declared ready and readAllBars aborts the whole build; if it exists but is stale, Bars can supply it and the stale input is silently aggregated. Compute coverage over the full union of week spans being considered (through the final week's end), then test a month-end week with missing and stale next-month D1.
There was a problem hiding this comment.
Fixed: added weekSpansForMonth, which computes the full union of a month's week spans up front (including a final week's spillover past monthEnd), and deriveAndPublish now queries D1 coverage over that full union rather than just [monthStart, monthEnd). A missing or invalid next-month D1 partition is now correctly visible to weekIsD1Ready and the boundary week is skipped rather than wrongly declared ready. (A genuinely stale, as opposed to missing/invalid, next-month partition isn't independently reachable through this particular coverage call, since deriveAndPublish's own D1 coverage query passes a nil raw-inventory lookup — the same 'cannot verify staleness' precedent already established elsewhere in this package — so I added Missing and Invalid regression tests, which are the non-Current statuses this query path can actually produce: TestBuild_DeriveSkipsBoundaryWeekWhenNextMonthD1Missing and TestBuild_DeriveSkipsBoundaryWeekWhenNextMonthD1Invalid.) Commit 3ea9620.
| CalendarVersion: calendarVersionCurrent, | ||
| BuiltAt: m.clock.Now(), | ||
| BarCount: len(bars), | ||
| Parent: &ParentRef{ |
There was a problem hiding this comment.
A W1 partition is not actually derived from only this same-month D1 revision: its boundary weeks can consume D1 bars from adjacent monthly partitions. Recording only the same-month parent means rebuilding an adjacent D1 partition will not make this W1 partition stale, even though its output may depend on the changed data; propagating only this parent's raw fingerprint has the same provenance gap. The explicit single-parent simplification therefore breaks the acceptance requirement for parent lineage/staleness. Please represent all contributing parent revisions (or define a deterministic composite parent revision/fingerprint) and have isStale compare the complete input set.
There was a problem hiding this comment.
Fixed: added combineParentLineage, which folds every D1 manifest that actually contributed a published bar (still exactly one in the common, non-boundary-spanning case — output unchanged from the original single-parent design) into one composite sha256: Revision/RawFingerprint. coverage.go's isStale now recomputes the identical composite from a stored Manifest's own LastBar/Span via the same w1SpansNextMonth rule deriveAndPublish uses to decide what to record, so the two can never disagree about which case applies. Added TestBuild_DeriveCombinesParentLineageAcrossMonthBoundary, which rebuilds only the contributing next-month D1 partition and confirms Coverage now reports the W1 partition Stale. Commit 3ea9620.
…CTOU race, cross-month D1 coverage/lineage Fixes six issues raised by Copilot and rustyeddy's review of #96: - aggregateBars: num.ParseRate instead of MustParseRate on runtime tick counts, propagating the error instead of risking a panic. - readAllBars: return nil (not a partial slice) on any non-EOF error, matching Bars' own no-partial-results-on-error contract. - Build no longer requires RawRoot unconditionally; the check moved into normalizeAndPublish, the only path that actually reads raw data, so a W1-only Plan (or a Plan with only Skipped raw actions) works with no RawRoot configured, per deriveActionsW1's own contract. - oanda.ReadPartitionSnapshot: one os.ReadFile producing both records and fingerprint atomically, replacing normalizeAndPublish's previous two-open ReadPartitionRecords+FingerprintPartition pattern, which admitted a window for Sync to replace the file in between and pair one revision's records with another's fingerprint. - deriveAndPublish's D1 coverage query now covers the full union of week spans (weekSpansForMonth), not just [monthStart, monthEnd), so a missing/invalid next-month D1 partition a boundary week spills into is correctly seen as not-ready instead of wrongly aborting the build. - Manifest.Parent's Revision/RawFingerprint are now a composite (combineParentLineage) over every D1 partition that actually contributed a published bar, not just the same-month one, so rebuilding a contributing next-month D1 partition now correctly marks the W1 partition stale (isStale recomputes the identical composite via the shared w1SpansNextMonth rule). Regression tests added for all six: oanda.TestReadPartitionSnapshot_* (including a -race concurrent-replace test), and marketdata's TestBuild_DeriveWorksWithNoRawRoot, TestBuild_PlanWithOnlySkippedActionsReportsThemWithoutRawRoot, TestBuild_DeriveSkipsBoundaryWeekWhenNextMonthD1Missing, TestBuild_DeriveSkipsBoundaryWeekWhenNextMonthD1Invalid, TestBuild_DeriveCombinesParentLineageAcrossMonthBoundary. marketdata coverage 92.2%, oanda coverage 91.9%. make check passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCR2bDEEGEsBfyu4hKu9f2
|
Addressed all six review findings (2 from Copilot, 4 from @rustyeddy) in commit 3ea9620 — replied individually on each thread with the specific fix. Summary:
New regression tests for all six (including a |
|
One convergence edge case remains after the otherwise solid review fixes:
Please make W1 planning reconsider a Current partition when it has W1 gaps, and schedule derivation once the complete D1 input for those week spans—including cross-month spillover—is available. A useful end-to-end regression would: build with February D1 absent, verify the boundary W1 gap; publish February D1; call |
deriveActionsW1 previously skipped every PartitionCoverageCurrent partition unconditionally, so a boundary week deriveAndPublish had to skip (its D1 input, spilling into the next month, was not yet available at build time) could never reconverge once that D1 data arrived: the partition's own recorded lineage reflects only what it did draw from, so nothing about its state changes on its own. deriveActionsW1 now reconsiders a Current partition specifically when Coverage's own W1-level Gaps overlap its month. The new w1CurrentPartitionNeedsDerive checks each overlapping gap's own D1 readiness via weekIsD1Ready (the identical per-week test deriveAndPublish itself applies at Build time), over a D1 Coverage queried through weekSpansForMonth's own coverageEnd so cross-month spillover is visible here too — rather than requiring the whole month's D1 input to be gapless, which the unchanged missing/invalid/ stale branches still use. Adds the requested end-to-end regression, TestPlan_W1ConvergesAfterBoundaryGapFillsIn: build with the boundary week's D1 absent, verify the gap; publish that D1 data; Plan again and verify it emits ActionDeriveCanonical for the already-Current partition; Build; verify the boundary bar is now present and the gap is gone. marketdata coverage 91.9%. make check passes. ADR-020 updated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KCR2bDEEGEsBfyu4hKu9f2
|
Fixed in commit 5fc235f. Added
|
What changed
Adds
Manager.Build, the canonical-build counterpart toSync(#80): it executes exactly theActionNormalizeCanonicalandActionDeriveCanonicalentries in aPlan, mirroringSync's own "only these actions" scope split (Buildnever runsActionDownloadRaw/ActionRepairRaw;Syncnever runs a canonical build action — each reports the other's actions in its ownSkipped).normalizeAndPublish(build_normalize.go): raw → same-interval canonical, wiring M2-08 Normalize and validate OANDA records into canonical Bars #76's normalizer to actual publication for the first time. AnySuspicious/Rejectedrecord anywhere in the partition aborts the whole call before anything is published (no partial publish);Incompleterecords (OANDA's owncompleteflag false) are excluded without aborting.deriveAndPublish+aggregateBars(resample.go): resamples canonical D1 into canonical W1 — the interval with no raw source. Re-checks D1 completeness per calendar week againstCoverage, independently of whatever range originally produced theAction, and leaves an unready week absent rather than aborting the whole month. The aggregation formula (OHLC/ticks/max-spread/tick-weighted avg-spread) is mined fromtrader-first-try/datamanager/candle_agg.go'saggregateWindow; legacy never actually built W1, so this is that formula applied to a genuinely new target, not transplanted code.oanda.FingerprintPartition: a targeted single-partition fingerprint (samesha256:<hex>formInspectproduces), used bynormalizeAndPublishforManifest.RawFingerprintwithout re-walking the whole raw archive.Manager.readAllBars: aBars-draining convenience wrapper. Needed becauseBarsrequires full coverage of its queried range whileCoveragedoes not —deriveAndPublishchecks D1 readiness once for the whole month viaCoverage, then loads bars per ready week only viareadAllBars.build_corpus_test.go(corpusbuild tag, operator-run, excluded from CI/make check, matching M2-07 Implement OANDA raw-archive inventory and integrity inspection #75'sfullarchiveprecedent): H4/D1 corpus comparison against OANDA-native partitions, and a one-time legacy candle-v2 comparison, both required by the issue's acceptance criteria.Why
Issue #81 (M2-13): materialize W1 (the one required interval with no raw provider source) via a canonical, DST-safe resampler, and validate derived intervals at corpus scale against OANDA-native data before any future candle-v2 deletion.
How it was tested
build_test.go/resample_test.go: full unit/integration coverage ofBuild, both action paths, cache invalidation, cancellation, and edge cases.marketdatapackage coverage 92.3%,oandapackage coverage 92.1%.make check(fmt, vet,test -raceacross the whole repo) is green.""and every corpus test skips itself). Two findings:readAllBars/Barsdoes), never mid-month.Documentation
docs/arch/adr-020-historic-data.org: newDONEsection covering both build paths, the mined aggregation formula, the same-month-keyParentconvention, and the corpus-run findings above.marketdata/doc.go: newManager.Buildsection.marketdata/internal/provider/oanda/doc.go: newFingerprintPartitionsection.🤖 Generated with Claude Code