feat: bitemporal holdings store + as_of query (#1463) - #1474
Conversation
Preserve holdings history across re-ingest and amendments so point-in-time reconstructions and backtest foundations stay safe (closes #1463). Co-authored-by: Cursor <cursoragent@cursor.com>
stranske
added
autofix
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
stranske
added
the
agent:retry
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds bitemporal columns and indexes to holdings, changes ingestion to append versions and supersede prior rows, adds current and as-of query helpers, and introduces acceptance tests for historical visibility, idempotency, no-look-ahead, and current-row behavior. ChangesBitemporal Holdings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant IngestFlow
participant HoldingsTable
participant FilingsTable
participant PointInTime
IngestFlow->>HoldingsTable: compare content hash
IngestFlow->>FilingsTable: resolve filing periods
IngestFlow->>HoldingsTable: supersede active rows and insert version
PointInTime->>FilingsTable: select latest filings before cutoff
PointInTime->>HoldingsTable: apply temporal visibility filters
HoldingsTable-->>PointInTime: return historical holdings
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Runner dispatch state for autofix on PR #1474. Do not edit. |
|
Autofix updated these files:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@alembic/versions/012_bitemporal_holdings.py`:
- Around line 23-44: Update the migration’s knowledge_time handling in the
holdings table alteration: add it as nullable initially without applying the
server default to existing rows, run the existing created_at-based backfill,
then alter knowledge_time to nullable=False while retaining CURRENT_TIMESTAMP as
the server default for future inserts. Preserve the existing superseded_at and
version changes.
- Around line 58-67: Update the v_current_holdings view creation in the
migration to use PostgreSQL-compatible CREATE OR REPLACE VIEW syntax instead of
CREATE VIEW IF NOT EXISTS, matching the existing schema.sql convention while
preserving the current SELECT and filtering logic.
In `@etl/point_in_time.py`:
- Around line 95-147: Update holdings_as_of’s filing-selection logic around
filing_rows and latest_by_period to apply knowledge-time visibility before
choosing the latest filing for each period. Exclude filings ingested after as_of
when has_knowledge is available, using the existing knowledge-time column and
parameter conventions, while preserving the filed_date cutoff and fallback
behavior for schemas without knowledge metadata.
In `@tests/test_bitemporal_holdings.py`:
- Around line 58-136: The test
test_holdings_as_of_preserves_versions_and_no_lookahead must cover an amendment
whose filed_date is on or before the as-of timestamp while its knowledge_time is
later. Add or adjust the amendment scenario so holdings_as_of queried before
that knowledge_time returns the prior filing version, then retain assertions
that the amended version appears after its knowledge_time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93bd8c83-82b0-43d2-bac7-82a9fa6cc5f4
📒 Files selected for processing (6)
alembic/versions/012_bitemporal_holdings.pyetl/edgar_flow.pyetl/ingest_flow.pyetl/point_in_time.pyschema.sqltests/test_bitemporal_holdings.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
etl/point_in_time.py (1)
39-61: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep holdings identifier handling consistent across both retrieval helpers.
The code advertises support for either
holding_idorid, but both query paths hard-codeh.holding_id, causing runtime failures onid-only schemas.
etl/point_in_time.py#L39-L61: derive the ordering column from the detected schema and use it in bothcurrent_holdingsSQL branches.etl/point_in_time.py#L139-L145: apply the same detected ordering column inholdings_as_of, or reject schemas withoutholding_idconsistently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etl/point_in_time.py` around lines 39 - 61, Make holdings ordering schema-aware in both current_holdings (etl/point_in_time.py:39-61) and holdings_as_of (etl/point_in_time.py:139-145): derive the ordering column as holding_id when available, otherwise id, and use it in each SQL ORDER BY. Keep identifier detection consistent across both helpers, or reject id-only schemas consistently if holdings_as_of cannot support them.
♻️ Duplicate comments (2)
alembic/versions/012_bitemporal_holdings.py (2)
36-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe
knowledge_timebackfill is still unreachable.Because the column is added as
NOT NULLwith aCURRENT_TIMESTAMPdefault, existing rows already have non-null values; thisWHERE knowledge_time IS NULLmatches nothing. Historical rows therefore lose their originalcreated_atknowledge time. Add the column nullable, backfill, then enforceNOT NULLand the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alembic/versions/012_bitemporal_holdings.py` around lines 36 - 40, Update the holdings migration around the knowledge_time column creation and backfill: add knowledge_time as nullable without the default, execute the existing created_at-based backfill for all historical rows, then enforce the NOT NULL constraint and CURRENT_TIMESTAMP default afterward.
54-59: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winUse a PostgreSQL-compatible view creation path.
CREATE VIEW IF NOT EXISTSis invalid PostgreSQL syntax, so this migration fails on PostgreSQL despite the SQLite/PostgreSQL compatibility requirement. Use a dialect branch (CREATE OR REPLACE VIEWfor PostgreSQL andCREATE VIEW IF NOT EXISTSfor SQLite), or another tested portable strategy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@alembic/versions/012_bitemporal_holdings.py` around lines 54 - 59, Update the v_current_holdings creation in the migration upgrade path to use a PostgreSQL-compatible dialect branch: CREATE OR REPLACE VIEW for PostgreSQL and CREATE VIEW IF NOT EXISTS for SQLite. Preserve the existing view definition and ensure the selected SQL executes correctly for each supported dialect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@etl/point_in_time.py`:
- Around line 39-61: Make holdings ordering schema-aware in both
current_holdings (etl/point_in_time.py:39-61) and holdings_as_of
(etl/point_in_time.py:139-145): derive the ordering column as holding_id when
available, otherwise id, and use it in each SQL ORDER BY. Keep identifier
detection consistent across both helpers, or reject id-only schemas consistently
if holdings_as_of cannot support them.
---
Duplicate comments:
In `@alembic/versions/012_bitemporal_holdings.py`:
- Around line 36-40: Update the holdings migration around the knowledge_time
column creation and backfill: add knowledge_time as nullable without the
default, execute the existing created_at-based backfill for all historical rows,
then enforce the NOT NULL constraint and CURRENT_TIMESTAMP default afterward.
- Around line 54-59: Update the v_current_holdings creation in the migration
upgrade path to use a PostgreSQL-compatible dialect branch: CREATE OR REPLACE
VIEW for PostgreSQL and CREATE VIEW IF NOT EXISTS for SQLite. Preserve the
existing view definition and ensure the selected SQL executes correctly for each
supported dialect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8668147c-71c3-4bdb-899a-7d5131ac7d63
📒 Files selected for processing (3)
alembic/versions/012_bitemporal_holdings.pyetl/point_in_time.pytests/test_bitemporal_holdings.py
|
Addressed the current CodeRabbit findings in fe1cc98. The migration now backfills knowledge_time before enforcing its default/NOT NULL constraint and uses a PostgreSQL-safe view statement; as-of filing selection excludes rows learned after the query instant; both helpers support id-only holdings tables; and the amended-filing test now exercises filed-before/learned-after visibility. Validation: pytest tests/test_bitemporal_holdings.py tests/test_config_migration_consistency.py -q (9 passed), ruff check, and an id-only SQLite smoke test. |
|
Workflow state fingerprint for Agents Verifier. Do not edit. |
stranske
added
verify:compare
|
Closer verifier-disposition audit: verifier run 30105537454 returned CONCERNS solely because it sampled |
|
Verifier reset executed by the closer lane: manual dispatch run 30160098574 ran the full compare path on this merged PR with green post-merge CI (it did not skip on unchanged state, because a workflow_dispatch changes the state fingerprint). It still returned CONCERNS, but the artifact shows the reason is |
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
DisagreementNo major disagreements detected. Unique Insights
🔍 LangSmith Traces |
1 participant
Closes #1463
Summary
knowledge_time,superseded_at,version,content_hash) with Alembic012+schema.sql+v_current_holdingsetl/point_in_time.py(holdings_as_of,current_holdings) for point-in-time reconstructionValidation
PYTHONPATH=. python3 -m pytest tests/test_bitemporal_holdings.py -qPYTHONPATH=. python3 -m pytest tests/test_ingest_flow.py::test_replace_holdings_rows_uses_postgres_transaction -qNotes
Foundation for backtest (#1464) and attribution (#1465). Large core-table migration — recommend owner review before merge.
Summary by CodeRabbit