chore(warehouse-sources): move the SCD2 writer into core/delta - #76067
Conversation
Phase 3 (PR 2) of the warehouse-sources untangling, after the maintenance split (#75978): the CDC-only SCD2 write path leaves DeltaTableHelper. - core/delta/scd2.py: Scd2DeltaWriter, a stateless wrapper over the table helper. The validity-interval column names are injected by the caller (the CDC load processor passes the columns cdc/batcher.py stamps), so the writer itself carries nothing CDC-specific and can be reused when CDC becomes a source-oriented ingress consumed by the scheduled sync. - core/delta/evolution.py: schema evolution (evolve_delta_schema) as a free function shared by the core write path and the SCD2 writer; kept out of delta/ops.py so the dlt import stays off the maintenance-only import path. - delta_merge_spill_kwargs moves to delta/ops.py; the pure-arrow helpers realign_decimal_buffers and first_per_pk_table move to core/arrow_utils (repartition.py stops importing them from the helper), all dropping their now-misleading leading underscores. No behavior changes; DeltaTableHelper shrinks by ~240 lines and its only remaining write concern is write_to_deltalake, which PR 3 extracts.
|
😎 This pull request was merged. |
|
Hey @estefaniarabadan! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
Prompt To Fix All With AI### Issue 1
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test/test_delta_table_helper.py:630
**Deduplication test is not collected**
The renamed `testfirst_per_pk_table_keep_modes` method lacks pytest's required `test_` prefix, so both parameterized keep-mode cases are silently skipped and regressions in first/last PK deduplication can pass CI.
```suggestion
def test_first_per_pk_table_keep_modes(self, _name, keep, expected_names):
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(warehouse-sources): move the SCD2 ..." | Re-trigger Greptile |
…ines/core/test/test_delta_table_helper.py Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
Note 🤖 stamphog review failed before producing a verdict Check the workflow run and re-apply the label to retry. |
There was a problem hiding this comment.
Mechanical, behavior-preserving extraction of the SCD2 Delta writer and shared helpers into new modules — verified the moved logic (decimal realignment, per-PK dedup, schema evolution, spill kwargs) is copied verbatim and the newly-parameterized valid_from/valid_to column names resolve to the same literals as before, so no behavior change. Author is on the owning team, tests moved with their code plus one new regression test for the exact redelivery/data-loss invariant called out, and the sole reviewer comment (missing test_ prefix) is already resolved in the current diff.
- Author wrote 4% of the modified lines and has 9 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from greptile-apps[bot], hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 524L, 7F substantive, 750L/11F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (750L, 11F, single-area, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ ed30f8c · reviewed head 06e5982 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/pipelines/pipeline_v3/load/processor.py |
33.3% | 789, 794 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/scd2.py |
93.5% | 118–120 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/arrow_utils.py |
95.5% | 1283 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30656545601 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
tasks |
██████████████░░░░░░ 70.3% |
33,203 / 47,255 |
signals |
████████████████░░░░ 81.4% |
25,289 / 31,055 |
cdp |
████████████████░░░░ 82.1% |
3,285 / 3,999 |
data_modeling |
█████████████████░░░ 85.7% |
7,716 / 9,008 |
notebooks |
█████████████████░░░ 86.0% |
7,794 / 9,060 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
cohorts |
█████████████████░░░ 87.5% |
6,393 / 7,309 |
exports |
██████████████████░░ 87.9% |
7,078 / 8,054 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 88.4% |
12,264 / 13,870 |
engineering_analytics |
██████████████████░░ 89.3% |
6,529 / 7,309 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
conversations |
██████████████████░░ 89.5% |
17,667 / 19,746 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
mcp_analytics |
██████████████████░░ 90.9% |
3,191 / 3,511 |
error_tracking |
██████████████████░░ 91.0% |
10,928 / 12,010 |
slack_app |
██████████████████░░ 91.1% |
9,664 / 10,610 |
marketing_analytics |
██████████████████░░ 91.2% |
12,092 / 13,265 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
product_analytics |
███████████████████░ 92.5% |
5,849 / 6,321 |
early_access_features |
███████████████████░ 92.6% |
1,287 / 1,390 |
ai_observability |
███████████████████░ 92.8% |
15,618 / 16,821 |
surveys |
███████████████████░ 93.1% |
5,771 / 6,197 |
posthog_ai |
███████████████████░ 93.2% |
1,326 / 1,422 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
web_analytics |
███████████████████░ 93.4% |
15,047 / 16,112 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
workflows |
███████████████████░ 94.1% |
7,251 / 7,707 |
endpoints |
███████████████████░ 94.2% |
8,655 / 9,192 |
review_hog |
███████████████████░ 94.6% |
8,101 / 8,563 |
skills |
███████████████████░ 94.6% |
3,158 / 3,337 |
logs |
███████████████████░ 95.4% |
10,487 / 10,987 |
experiments |
███████████████████░ 95.5% |
26,047 / 27,287 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
feature_flags |
███████████████████░ 96.4% |
17,488 / 18,144 |
replay_vision |
███████████████████░ 96.4% |
16,081 / 16,679 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
customer_analytics |
███████████████████░ 97.1% |
10,463 / 10,777 |
warehouse_sources |
███████████████████░ 97.3% |
359,891 / 369,896 |
data_catalog |
████████████████████ 97.8% |
2,589 / 2,648 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
Phase 3 (PR 3) of the warehouse-sources untangling, after #75978 and #76067: the core write/merge path and commit-metadata idempotency move to core/delta/writer.py, leaving DeltaTableHelper as a pure table access and lifecycle object (~270 lines: cached open, corruption detection, reset, file listing, first-sync state). DeltaWriter is a stateless wrapper over the helper, mirroring DeltaMaintenance and Scd2DeltaWriter: write_to_deltalake becomes DeltaWriter.write, together with the deltalite canary (moved verbatim, mid-rollout), batch dedupe, schema-mismatch fallback, and the idempotency pair has_commit_with_metadata / has_batch_been_committed. Tag-write and tag-read stay together deliberately: only the terminal commit of a multi-commit write may carry the (run_uuid, batch_index) tag, and that invariant spans both halves. _commit_matches becomes the module-level commit_matches, shared with the SCD2 tagging test. Callers updated: the v2 pipeline chunk write, the v3 load processor, the CDC companion seeding in common/load.py, and the redelivery fallback in load/idempotency.py (which now wraps the helper in a DeltaWriter). Debug log prefixes change from 'write_to_deltalake:' to 'write:' with the rename; no behavior changes otherwise.
Problem
Second step of splitting
DeltaTableHelper(after the maintenance extraction in #75978): the SCD Type 2 write path is CDC-only — its single caller is the v3 CDC load processor — yet it lives inside the shared helper alongside the core write path, dragging CDC semantics into every reader of that file.Changes
core/delta/scd2.py:Scd2DeltaWriter, a stateless wrapper over the table helper carrying the close-existing-then-append SCD2 write, including the invariant that only the terminal append commit is tagged with(run_uuid, batch_index)— tagging the intermediate close merge would make Kafka redelivery skip the append after a mid-write crash. The validity-interval column names are injected by the caller (the processor passes the columnscdc/batcher.pystamps), so the writer itself carries nothing CDC-specific and can be reused when CDC becomes a source-oriented ingress consumed by the normal scheduled sync.core/delta/evolution.py: schema evolution (evolve_delta_schema) as a free function shared by the core write path and the SCD2 writer. It's a separate module rather than part ofdelta/ops.pyso thedltimport stays off the maintenance-only import path chore(warehouse-sources): split delta maintenance out of DeltaTableHelper #75978 cleaned up.delta_merge_spill_kwargsmoves todelta/ops.py; the pure-arrow helpersrealign_decimal_buffersandfirst_per_pk_tablemove tocore/arrow_utils.py(andrepartition.pystops importing a private from the helper). All three drop their now-misleading leading underscores.No behavior changes.
DeltaTableHelpershrinks by ~240 lines; its only remaining write concern iswrite_to_deltalake, which the next PR extracts into aDeltaWriter.How did you test this code?
Automated only (no manual testing):
delta/test/test_scd2.py, the spill-kwargs settings-wiring test todelta/test/test_ops.py; the realign/first-per-pk tests stay with the write-path suite and now import fromarrow_utils.test_only_terminal_append_commit_carries_metadata: asserts against real delta history that the close merge stays untagged and only the append carries the idempotency metadata — the data-loss-on-redelivery regression described above had no coverage.Scd2DeltaWriter.writeis never reached when ownership is lost.ruffand repo-wideuv run mypy --cache-fine-grained .(clean).Automatic notifications
Docs update
No user-facing or documented-workflow changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code, continuing the agreed
DeltaTableHelpersplit plan (maintenance → SCD2 → writer → rename).Skills invoked: /writing-tests.
Decisions: the writer takes required
valid_from_column/valid_to_columnparameters instead of importing the CDC batcher's constants, which keepscdc/out ofcore/delta's import graph while leaving one source of truth at the call site; schema evolution went into its own module purely to keepdltoff the maintenance import path.Commit landed via GraphQL
createCommitOnBranch(server-side signing), with the remote tree verified equal to the locally tested commit.