Fix SEC decoding and financial fact grain - #2
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR preserves financial period grain in the mart, adds period-grain validation, enables gzip and deflate response decoding in the SEC client, adds compressed-response tests, and runs Ruff formatting checks in CI and local validation. ChangesFinancial period grain
SEC response decoding
Format validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SecCompanyFactsClient
participant urlopen
participant HTTPResponse
participant _decompress_body
participant _decode_payload
SecCompanyFactsClient->>urlopen: request company facts
urlopen-->>SecCompanyFactsClient: compressed response and Content-Encoding
SecCompanyFactsClient->>HTTPResponse: read raw bytes
SecCompanyFactsClient->>_decompress_body: decompress response bytes
_decompress_body-->>SecCompanyFactsClient: return decompressed bytes
SecCompanyFactsClient->>_decode_payload: decode UTF-8 JSON
_decode_payload-->>SecCompanyFactsClient: return company facts payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_pipeline.py (1)
20-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert period/value pairs rather than independent sets.
The current assertions pass if the two values become associated with the wrong start dates. Assert tuples to preserve the fixture’s quarterly-vs-YTD semantic contract.
Proposed test tightening
- assert {row["period_start_date"] for row in rows} == {"2026-01-01", "2026-07-01"} - assert {row["value"] for row in rows} == {250000, 750000} + assert { + (row["period_start_date"], row["value"]) + for row in rows + } == { + ("2026-01-01", 750000), + ("2026-07-01", 250000), + }🤖 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 `@tests/test_pipeline.py` around lines 20 - 21, Update the assertions in the test around the rows collection to compare paired (period_start_date, value) tuples rather than independent sets. Verify the expected associations are 2026-01-01 with 250000 and 2026-07-01 with 750000, preserving the fixture’s quarterly-versus-YTD contract.dbt/tests/test_fct_financial_facts_preserves_period_grain.sql (1)
1-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare period identities, not only counts.
A mart with two incorrect start dates still passes when its distinct count matches the source. Compare the distinct keyed periods symmetrically so substitutions and unexpected extras fail the test.
Proposed test tightening
with source_periods as ( - select + select distinct cik, fact_namespace, fact_name, unit, accession_number, period_end_date, - count( - distinct coalesce(cast(period_start_date as varchar), '__instant__') - ) as period_count + coalesce(cast(period_start_date as varchar), '__instant__') as period_start_key from {{ ref('stg_financial_facts') }} - group by all ), mart_periods as ( - select + select distinct cik, fact_namespace, fact_name, unit, accession_number, period_end_date, - count( - distinct coalesce(cast(period_start_date as varchar), '__instant__') - ) as period_count + coalesce(cast(period_start_date as varchar), '__instant__') as period_start_key from {{ ref('fct_financial_facts') }} - group by all ) -select source_periods.* -from source_periods -left join mart_periods - using (cik, fact_namespace, fact_name, unit, accession_number, period_end_date) -where source_periods.period_count != coalesce(mart_periods.period_count, 0) +( + select * from source_periods + except + select * from mart_periods +) +union all +( + select * from mart_periods + except + select * from source_periods +)🤖 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 `@dbt/tests/test_fct_financial_facts_preserves_period_grain.sql` around lines 1 - 35, Update the test query to compare the actual distinct period identities between stg_financial_facts and fct_financial_facts, using the existing grouping keys plus period_start_date (with the instant-date sentinel). Perform a symmetric comparison so missing source periods and unexpected mart periods both produce failures, rather than comparing only period_count values.tests/test_sec_client.py (1)
34-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover truncated-body retry behavior.
This only tests successful decompression. Add a regression case where the first compressed response is truncated and a subsequent response is valid, asserting
urlopenis called twice.🤖 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 `@tests/test_sec_client.py` around lines 34 - 60, Extend test_live_client_decodes_compressed_responses with a retry regression case using sequential fake responses: return a truncated compressed body first, then a valid compressed payload, and assert fetch_companyfacts succeeds with urlopen called twice. Preserve coverage for both gzip and deflate encodings and configure retries to allow the second attempt.
🤖 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.
Nitpick comments:
In `@dbt/tests/test_fct_financial_facts_preserves_period_grain.sql`:
- Around line 1-35: Update the test query to compare the actual distinct period
identities between stg_financial_facts and fct_financial_facts, using the
existing grouping keys plus period_start_date (with the instant-date sentinel).
Perform a symmetric comparison so missing source periods and unexpected mart
periods both produce failures, rather than comparing only period_count values.
In `@tests/test_pipeline.py`:
- Around line 20-21: Update the assertions in the test around the rows
collection to compare paired (period_start_date, value) tuples rather than
independent sets. Verify the expected associations are 2026-01-01 with 250000
and 2026-07-01 with 750000, preserving the fixture’s quarterly-versus-YTD
contract.
In `@tests/test_sec_client.py`:
- Around line 34-60: Extend test_live_client_decodes_compressed_responses with a
retry regression case using sequential fake responses: return a truncated
compressed body first, then a valid compressed payload, and assert
fetch_companyfacts succeeds with urlopen called twice. Preserve coverage for
both gzip and deflate encodings and configure retries to allow the second
attempt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 026ce211-08d6-4d5d-b2d4-f679a16aa7d9
📒 Files selected for processing (8)
.github/workflows/ci.ymldbt/models/marts/fct_financial_facts.sqldbt/tests/test_fct_financial_facts_preserves_period_grain.sqlscripts/validate.shsrc/ledger/sec_client.pytests/fixtures/CIK0000320193.jsontests/test_pipeline.pytests/test_sec_client.py
1 participant
Summary
period_start_datein the financial fact grain and deterministic identifierAcceptance criteria
Scope
No cloud deployment, live SEC request, repository visibility change, or unrelated refactor is included.
Summary by CodeRabbit