chore(data-warehouse): remove the deltalite write rollout flag - #106874
Conversation
deltalite is the write path for every keyed incremental merge now, so the per-schema rollout gate only costs. It was evaluated inside _write_via_deltalite on the hot path before every merge: two Postgres queries (Team, then ExternalDataSchema to resolve source_type) plus a feature_enabled call with only_evaluate_locally=False. Production measurement puts the deltalite write at ~3.8s average per batch with half of all writes moving under 30 rows, so per-batch fixed cost is what governs drain time for a backlogged schema. Delete the flag module and its gate. The delta-rs MERGE fallback is unchanged: deltalite failing still falls back, so this only removes a choice that was already always the same. Removing the MERGE fallback itself is a separate change.
|
😎 Merged successfully - details. |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🤖 CI report
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughKeyed incremental merges now attempt deltalite without checking a feature flag. If the upsert fails before commit, the writer returns Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Google Ads rows with NULL components in configured keys can be duplicated on repeated imports, making warehouse results incorrect and growing storage. Keep those batches on the NULL-safe MERGE path before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Keyed incremental merges now try deltalite without a per-schema switch. Failed attempts can still fall back, but that fallback cannot redirect a write that succeeds and produces incorrect data. The previous rollout state and an alternative way to disable deltalite have not been established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/writer.py-159-179 (1)
159-179: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep post-commit handle refresh failures out of the fallback path.
DeltaLiteTable.upsertcommits withCommitBuilder.build(...).awaitand then callsself.reload(py)?. A reload error after the durable commit reachesDeltaWriter._write_via_deltalite's fallback handler, which returnsFalseand runs the delta-rs MERGE. This can duplicate nullable primary-key rows: deltalite treats NULL keys as non-matching and inserts them, while the fallback predicate usesIS NOT DISTINCT FROM. The already-committed extra row cannot be removed by that MERGE.Make the handle reload best-effort after the commit.
Suggested fix
- self.reload(py)?; + // The upsert is already durable. A handle-refresh failure must not report + // the committed write as a failure to the caller. + let _ = self.reload(py);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a7dc9a61-f572-4a98-a614-855e51823a8c
📒 Files selected for processing (6)
products/data_warehouse/backend/s3_proxy.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/auto_widen_resync.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_writer.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/writer.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/deltalite_write.pyproducts/warehouse_sources/backend/temporal/data_imports/pipelines/core/test/test_deltalite_write.py
💤 Files with no reviewable changes (2)
- products/warehouse_sources/backend/temporal/data_imports/pipelines/core/test/test_deltalite_write.py
- products/warehouse_sources/backend/temporal/data_imports/pipelines/core/deltalite_write.py
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
…llback `_write_via_deltalite` no longer gates on a rollout flag, and `deltalite` is a real dependency in CI (unlike the old flag, which failed closed under test), so every primary-keyed `write()` call in this file was actually driving the real `deltalite` package instead of the delta-rs MERGE path most of these tests assert on. Default `_write_via_deltalite` to the MERGE fallback for every test in the module, except `TestDeltaliteWritePath` (drives the real method / fakes the `deltalite` module itself) and `TestNullabilityDriftGuardOrder` (already sets this mock explicitly in both directions).
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
This removes the rollout flag so deltalite becomes the write path for 100% of keyed incremental merges (previously a partial, killable rollout) — a production data-integrity change. A bot reviewer (@veria-ai) flagged an unresolved, substantive concern on writer.py: deltalite treats NULL primary-key components as never matching while the delta-rs MERGE fallback is NULL-safe, so once deltalite is universal, NULL-keyed source rows can accumulate as unbounded duplicate inserts. The PR description doesn't address this, and the review summary explicitly reports 0 issues fixed.
- Author wrote 71% of the modified lines and has 35 merged PRs in these paths (familiarity STRONG).
- Unresolved inline comment from @veria-ai on writer.py:169 — NULL primary keys become an unbounded insert path now that deltalite (which is not NULL-safe on primary-key matching) handles every keyed merge instead of a partial rollout; not addressed in the diff or description.
- Cross-team change (touches team-managed-warehouse's s3_proxy.py) in a risky, production write-path area; independent assurance would otherwise come from author's STRONG familiarity on the warehouse-sources side, but the open correctness concern blocks approval regardless.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 98L, 4F substantive, 221L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (221L, 6F, single-area, chore) |
| stamphog 2.1.0 | .stamphog/policy.yml @ c146704 · reviewed head c146704 |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_writer.py-120-120 (1)
120-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAnnotate the generator fixture return type.
This fixture yields only
None, and the project requires annotations on every Python signature. ImportGeneratorfromcollections.abc;Generator[None]is the repository-supported form.Suggested fix
+from collections.abc import Generator ... -def _default_deltalite_to_merge_fallback(request: pytest.FixtureRequest): +def _default_deltalite_to_merge_fallback( + request: pytest.FixtureRequest, +) -> Generator[None]:
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: b7524afe-05e8-4cfe-a6bf-58504b74fd2c
📒 Files selected for processing (1)
products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_writer.py
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
…ltalite path `_write_via_deltalite` no longer gates on a rollout flag, and `deltalite` is a real dependency in CI (unlike the old flag, which failed closed under test), so tests that call `write()`/the pipeline with primary keys were driving the real `deltalite` package instead of the delta-rs MERGE path they actually assert on. - `delta/test/test_writer.py`: default `_write_via_deltalite` to the MERGE fallback for every test in the module, except `TestDeltaliteWritePath` (drives the real method / fakes the `deltalite` module itself) and `TestNullabilityDriftGuardOrder` (already sets this mock explicitly in both directions). - `tests/e2e/test_end_to_end.py`: `test_partition_folders_delta_merge_called_with_partition_predicate` asserts the delta-rs MERGE was called with a specific predicate; force the fallback there too so deltalite doesn't silently take the write instead.
|
Status update from this pass:
The remaining Needed from a human: either re-add the 🦉 via talyn.dev |
|
/trunk merge |
|
/trunk merge |
There was a problem hiding this comment.
Approved.
This touches the data-warehouse write path, but the author has STRONG familiarity with these exact files, the deleted flag was already fully rolled out (this just removes dead-weight per-batch overhead), and the current head has a clean CodeRabbit pass plus an explicit "no open concerns" from the security bot — the one substantive data-integrity comment raised earlier concerns pre-existing Rust behavior this PR doesn't touch, and was addressed/resolved. The one cross-team file (s3_proxy.py) is a trivial docstring edit.
- Author wrote 71% of the modified lines and has 42 merged PRs in these paths (familiarity STRONG).
- A generator test fixture still lacks the return-type annotation CodeRabbit suggested — cosmetic only, not blocking.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 98L, 4F substantive, 226L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (226L, 7F, single-area, chore) |
| stamphog 2.2.0 | .stamphog/policy.yml @ 0ef03a1 · reviewed head 0ef03a1 |
fuziontech
left a comment
fuziontech
left a comment
There was a problem hiding this comment.
Automated review generated by an automated review agent on behalf of @fuziontech.
The diff cleanly removes the rollout gate and updates the affected tests. Focused validation passed: uv run pytest -q products/warehouse_sources/backend/temporal/data_imports/pipelines/core/delta/test/test_writer.py (58 passed), plus compile and diff checks. The focused e2e test could not initialize because the configured db hostname was unavailable in this environment.
Non-blocking follow-up: a few comments/docstrings still describe this as a “phase 2 canary” or say fallback occurs when deltalite is “enabled”; they could be refreshed separately for terminology consistency.
APPROVE
Problem
deltalite is the write path for every keyed incremental merge, so the per-schema rollout flag that chose it no longer chooses anything — it just costs.
The gate ran inside
_write_via_deltalite, on the hot path before every merge:Team.objects.get, thenExternalDataSchema.objects.select_related("source").getto resolvesource_type, thenposthoganalytics.feature_enabled(..., only_evaluate_locally=False). Two Postgres queries and a flags evaluation per batch, to reach the same answer every time.That matters because per-batch fixed cost is what governs how fast a backlogged schema drains — batches for one
(team_id, schema_id)are processed strictly serially. Measured in prod-us over 600 samples, the deltalite write averages 3.8s per batch, and half of all writes move fewer than 30 rows. Anything paid once per batch is paid again for every batch in a backlog.Changes
deltalite_write.pyand its test module._write_via_deltaliteno longer evaluates a flag, so every keyed incremental merge reaches deltalite directly.is_deltalite_write_enabledin unrelated modules that cited it as the pattern to copy.The delta-rs MERGE fallback is unchanged. deltalite failing before commit still falls back to the MERGE, so this removes a choice that was already always the same — it does not remove the safety net. Removing the MERGE fallback is a separate, larger change.
How did you test this code?
Automated only. No manual run against a live pipeline.
core/delta/suite: 201 passed, 9 failed. All 9 failures are pre-existing, inTestGetDeltaTableUnrecoverableErrorsintest_table.py, a file this PR does not touch. I confirmed that by running the same file in a separate worktree that has none of these changes and seeing the identical 9 failures.test_writer.pyin full: 58 passed.ruff check/ruff format --checkclean; the pre-commit hook'sty checkpassed.One existing test needed a real change rather than a mechanical one.
test_merge_fallback_raises_reset_signal_on_null_in_non_nullablereached the delta-rs MERGE by relying on the flag evaluation failing closed in the test environment — an implicit dependency, not a stated one. With the flag gone, deltalite handled the batch and the guard correctly did not fire, so the test failed.The coverage is still worth having: the MERGE fallback still exists, and it would still silently store nulls under a non-nullable schema without that guard. So the test now forces the fallback explicitly by patching
_write_via_deltaliteto returnFalse, which is what its siblingtest_deltalite_write_bypasses_the_reset_signalalready does in the opposite direction. The regression it catches is unchanged; only the way it reaches the path is now stated rather than incidental.Not checked: behaviour under a real deltalite failure in production. The fallback path is exercised by tests, not by an observed live failure.
Release status
The
data-warehouse-deltalite-writeflag can be deleted from the flag UI once this is deployed.Automatic notifications
Docs update
None. No user-facing behaviour, API, or documented workflow changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5
Skills invoked:
/writing-pr-descriptions,/writing-tests.This came out of profiling why V3 batches take ~8s each. A phase breakdown from 600 production log samples showed the deltalite write dominating, which made the per-batch flag evaluation in front of it worth deleting rather than caching.
No duplicate:
gh pr list --state open --search "deltalite flag"found nothing related.CodeRabbit CLI is not installed on this machine, so this PR opens without a local review pass.
Production metrics and structured logs informed this. No customer identifiers, team ids, or table contents appear in the code, the tests, or this description.