chore(warehouse-sources): split delta maintenance out of DeltaTableHelper - #75978
Conversation
…lper Phase 3 (PR 1) of the warehouse-sources untangling: DeltaTableHelper carried four jobs in one 1,200-line file. This extracts the maintenance concern and the shared primitives into a new pipelines/core/delta/ package: - delta/errors.py: the transient-error classifiers, moved verbatim - delta/ops.py: execute_with_conflict_retry as a free function, shared by the write merges, the SCD2 close merge, and compaction - delta/maintenance.py: DeltaMaintenance, a stateless wrapper over the helper holding compact/vacuum/fragmentation and the vacuum-watermark cadence DeltaMaintenance.run_scheduled now owns watermark persistence: both call sites (the pre-write defensive pass in each pipeline and the CDC post-load pass) previously duplicated the read-watermark/run/persist block with subtly divergent transient-error checks. The narrower is_transient_object_store_error check on the post-load paths is unified to is_transient_maintenance_error, so the concurrent-maintenance file race and app-DB connection blips stop minting error-tracking issues there too. extract.run_pre_write_defensive_compact is deleted (both pipelines call run_scheduled directly) and the unused vacuum_table method is dropped. The dlt import stays with the writer, so maintenance-only callers no longer pay for it. No behavior changes beyond the error-classification unification above.
|
😎 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 |
|
Reviews (1): Last reviewed commit: "chore(warehouse-sources): split delta ma..." | Re-trigger Greptile |
pytest imports __init__-less test dirs as top-level modules, so the new delta/test/test_errors.py collided with sources/postgres/cdc/tests/test_errors.py and aborted collection for every warehouse-sources CI shard. Rename it to test_delta_errors.py. Also drop the redundant bare `import deltalake` in delta/errors.py (`import deltalake.exceptions` already binds the name).
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta_table_helper.py |
85.7% | 294, 298 |
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/maintenance.py |
95.8% | 82, 143–144, 220 |
🤖 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 30651420380 -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,293 / 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.8% |
6,181 / 7,040 |
exports |
██████████████████░░ 87.9% |
7,078 / 8,054 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
data_warehouse |
██████████████████░░ 88.4% |
12,192 / 13,798 |
engineering_analytics |
██████████████████░░ 89.3% |
6,529 / 7,309 |
conversations |
██████████████████░░ 89.5% |
17,667 / 19,746 |
dashboards |
██████████████████░░ 89.5% |
5,989 / 6,693 |
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 |
web_analytics |
███████████████████░ 93.3% |
14,911 / 15,976 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
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,846 / 369,850 |
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 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.
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
DeltaTableHelpercarries four jobs in one ~1,200-line file: the core write/merge path, the CDC SCD2 writer, maintenance (compact/vacuum), and commit-metadata idempotency.Every shared change lands in a file that also carries the other three concerns, and the maintenance watermark persistence was duplicated across two callers (
common/extract.pyandcommon/load.py) with subtly divergent transient-error checks.This is phase 3 (PR 1 of 4) of the warehouse-sources pipeline untangling, following the kernel extraction (#74352) and the post-load unification (#74671, #74746).
Changes
New
pipelines/core/delta/package:errors.pyholds the transient-error classifiers, moved verbatim.ops.pyturns_execute_with_conflict_retryinto a free function shared by the write merges, the SCD2 close merge, and compaction.maintenance.pyholdsDeltaMaintenance, a stateless wrapper over the table helper with compact/vacuum/fragmentation and the vacuum-watermark cadence.DeltaMaintenance.run_schedulednow owns watermark persistence.Both call sites (the pre-write defensive pass in the v2 and v3 pipelines, and the CDC post-load pass) previously duplicated the read-watermark/run/persist block, including the watermark-key choice for the
_cdccompanion table.extract.run_pre_write_defensive_compactis deleted and both pipelines callrun_scheduleddirectly.Note
One deliberate behavior change: the post-load maintenance paths used the narrower
is_transient_object_store_errorcheck while the pre-write path usedis_transient_maintenance_error.They are unified on the broader classifier, so the concurrent-maintenance file race and app-DB connection blips stop minting error-tracking issues on the post-load paths too.
Also dropped the unused
vacuum_tablemethod, and thedltimport now stays with the writer so maintenance-only callers no longer pay for it.How did you test this code?
Automated only (no manual testing):
delta/test/test_errors.py, conflict-retry tests todelta/test/test_ops.py, compaction/vacuum/cadence tests todelta/test/test_maintenance.py.TestRunScheduledcovers the consolidated watermark ownership: companion vs snapshot watermark-key selection (sharing a key corrupts both cadences), persistence gating (a lost watermark means the table never vacuums), partition-count fallback resolution, and the never-raise contract. These replace the equivalent caller-level tests that were deleted fromtest_extract.pyandtest_load.py.test_load.pynow asserts only the orchestration load still owns: which maintenance flavor runs per schema kind, and that a compaction failure doesn't stop post-load.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 following an agreed refactoring plan for splitting
DeltaTableHelper(writer, SCD2 writer, maintenance, table access).Skills invoked: /writing-tests.
Decisions: maintenance classes are stateless wrappers constructed at the call site so every existing signature keeps threading one object (the helper, which holds the cached table handle and
is_first_sync); commit-metadata idempotency deliberately stays with the writer for a later PR since tag-write and tag-read form one contract; the error-classification unification was chosen over preserving the narrower post-load check because the divergence looked like drift, not intent.Commit landed via GraphQL
createCommitOnBranch(server-side signing), with the remote tree verified equal to the locally tested commit.