Move in-memory history to per-entity-table storage - #1249
Closed
DZakh wants to merge 3 commits into
Closed
Conversation
Replace `inMemoryStoreEntityUpdate.history` with a single `InMemoryTable.Entity.t.history` array shared by all rows of an entity. Each row tracks its slot via `historyIndex` so a same-checkpoint write replaces in place, preserving the existing dedup invariant. History is now always populated in memory; the `shouldSaveHistory` flag only gates the Postgres flush (DB behavior unchanged) and is dropped from the context-param threading where it was no longer used. ClickHouse now writes the full table-level history, not only each row's `latestChange`, regardless of the flag. Rollback-diff replays skip the history buffer (`historyIndex = -1`) since they restore from existing history rows.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the threaded shouldSaveHistory flag and migrates per-update history arrays to a shared table-level history buffer with per-row historyIndex; updates handler APIs, in-memory store, persistence writers, ClickHouse binding, serialization, and tests. ChangesHistory Parameter Removal and Index-Based Tracking Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/envio/src/TestIndexer.res`:
- Line 162: The expression pushes parsedEntity->Utils.magic without an explicit
cast; update the call in the entityChange.sets->Array.push(...) expression so
Utils.magic is used with an explicit annotation, e.g. change
parsedEntity->Utils.magic to parsedEntity->(Utils.magic: inputType =>
outputType) using the correct inputType and outputType for parsedEntity and the
target set element type in TestIndexer.res; ensure the annotated cast matches
the expected types so the push compiles.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 779f58c9-093a-41d1-89f8-9620d5d2bf99
📒 Files selected for processing (15)
packages/envio/src/EventProcessing.respackages/envio/src/GlobalState.respackages/envio/src/InMemoryStore.respackages/envio/src/InMemoryTable.respackages/envio/src/Internal.respackages/envio/src/Persistence.respackages/envio/src/PgStorage.respackages/envio/src/Sink.respackages/envio/src/TestIndexer.respackages/envio/src/TestIndexerProxyStorage.respackages/envio/src/UserContext.respackages/envio/src/bindings/ClickHouse.resscenarios/test_codegen/test/EventOrigin_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/rollback/Rollback_test.res
💤 Files with no reviewable changes (3)
- scenarios/test_codegen/test/EventOrigin_test.res
- scenarios/test_codegen/test/helpers/MockIndexer.res
- packages/envio/src/UserContext.res
| entityChanges->Dict.set(entityName, change) | ||
| change | ||
| } | ||
| entityChange.sets->Array.push(parsedEntity->Utils.magic)->ignore |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add explicit cast annotation for Utils.magic.
Line 162 uses Utils.magic without the required input/output type annotation.
♻️ Suggested fix
- entityChange.sets->Array.push(parsedEntity->Utils.magic)->ignore
+ entityChange.sets->Array.push(parsedEntity->(Utils.magic: Internal.entity => unknown))->ignoreAs per coding guidelines, "When using Utils.magic for type casting in ReScript, always add explicit type annotations: value->(Utils.magic: inputType => outputType)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entityChange.sets->Array.push(parsedEntity->Utils.magic)->ignore | |
| entityChange.sets->Array.push(parsedEntity->(Utils.magic: Internal.entity => unknown))->ignore |
🤖 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 `@packages/envio/src/TestIndexer.res` at line 162, The expression pushes
parsedEntity->Utils.magic without an explicit cast; update the call in the
entityChange.sets->Array.push(...) expression so Utils.magic is used with an
explicit annotation, e.g. change parsedEntity->Utils.magic to
parsedEntity->(Utils.magic: inputType => outputType) using the correct inputType
and outputType for parsedEntity and the target set element type in
TestIndexer.res; ensure the annotated cast matches the expected types so the
push compiles.
Add optional `isRollbackDiff: bool` to `Change.Set` and `Change.Delete`. `InMemoryTable.Entity.set` reads the flag directly from the change to decide whether to record it in the table-level history buffer, and the Postgres flush reads it from `update.latestChange` to gate backfill. Drops the previous `containsRollbackDiffChange` row-level flag and the `~containsRollbackDiffChange` argument that propagated it forward. A user write following a rollback-diff replay no longer inherits the flag, so backfill is computed for that entity; the backfill query is idempotent so this is a no-op when history rows already exist, and records the pre-write state when they don't. Rollback test snapshots updated for the extra checkpoint=0 backfill rows that now appear after rollback + user write.
Contributor
|
Actionable comments posted: 0 |
…-XTQUN # Conflicts: # packages/envio/src/InMemoryTable.res
Contributor
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
historyarray offinMemoryStoreEntityUpdate(per-row) and ontoInMemoryTable.Entity.t(per entity table). Each row tracks its slot viahistoryIndexso a second write at the same checkpoint replaces in place — same dedup invariant as before, just keyed against a shared array.shouldSaveHistorygate at write time; history is always populated in memory. The flag is still respected at the Postgres flush (DB behavior unchanged) and removed from the context-param threading where it was no longer used.latestChange, regardless of the flag.historyIndex = -1) since they restore from existing history rows.Test plan
pnpm rescriptclean inpackages/envioandscenarios/test_codegenpnpm vitest run test/rollback/Rollback_test.res.mjs— 15/15pnpm vitest runforE2E_test,WriteRead_test,lib_tests/EntityHistory_test,lib_tests/PgStorage_test,lib_tests/ClickHouse_test,EventOrigin_test— 65 passed, 2 skippedhttps://claude.ai/code/session_01Xo1n3gN3az2PSH81vVa3Va
Generated by Claude Code
Summary by CodeRabbit
Refactor
Bug Fixes
Tests