Refactor index system to use EntityFilter instead of TableIndices - #1306
Conversation
Move FieldValue comparison logic into EntityFilter and add toString (stable in-memory cache key) and matches (evaluates a filter against an entity) so the single-field TableIndices module is no longer needed. - InMemoryTable keys its in-memory indices by EntityFilter.toString with a single flat dict instead of the per-field two-level structure - LoadLayer.loadByField becomes loadByFilter and passes the filter straight to storage.loadOrThrow - UserContext builds EntityFilter leaves directly; _in still decomposes into per-value Eq loads and _gte/_lte into Eq + Gt/Lt to keep per-value memoization - TestIndexer reuses EntityFilter.matches after parsing leaf values with the field schemas https://claude.ai/code/session_01QN8cfbDEtGeoSGRxSXirpt
|
Warning Review limit reached
More reviews will be available in 15 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors entity loading and indexing from field/operator-based index hashing to EntityFilter-based evaluation. EntityFilter gains FieldValue comparison and filter stringification; InMemoryTable switches from field/operator indices to filter tracking; LoadLayer migrates to filter-driven batch loading; UserContext and TestIndexer adopt the new filter-based APIs; and the LoadLayer test suite is updated accordingly. ChangesFilter-based Indexing Refactor
Possibly related PRs
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/envio/src/db/EntityFilter.res (1)
41-53: 💤 Low valueVerify behavior of
gt/ltfor mismatched orNonevalues.The fallback
a > banda < bcomparisons onoptiontypes rely on OCaml's polymorphic comparison. When comparingNonewithSome(_)or values of different underlying types (e.g.,Some(Int(1))vsSome(String("x"))), the result may be unintuitive or non-deterministic across JS engines. If such comparisons are expected at runtime, consider explicit handling.🤖 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/db/EntityFilter.res` around lines 41 - 53, The gt/lt functions currently fall back to polymorphic comparisons (a > b / a < b) which can yield unintuitive results for None vs Some or mismatched Some types; update gt and lt to explicitly handle option shapes: add cases for (None, None), (None, Some(_)) and (Some(_), None) and a clear policy for mismatched Some(x)/Some(y) (e.g., return false, raise an error, or coerce/compare only when types match), keeping the existing BigDecimal branch for (Some(BigDecimal...), Some(BigDecimal...)); reference the gt and lt functions and the BigDecimal pattern to locate where to add these explicit patterns.
🤖 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/db/EntityFilter.res`:
- Around line 76-77: The In branch is casting the entire array to FieldValue.t
which is incorrect; update the In({fieldName, fieldValue}) handler to iterate
over the array (fieldValue), for each element call FieldValue.castFrom(...) and
then FieldValue.toString(...) and join the results into a bracketed,
comma-separated string so the returned template uses
`${fieldName}:In:[elem1,elem2,...]`; reference the In pattern and the
FieldValue.castFrom / FieldValue.toString helpers when making this change.
---
Nitpick comments:
In `@packages/envio/src/db/EntityFilter.res`:
- Around line 41-53: The gt/lt functions currently fall back to polymorphic
comparisons (a > b / a < b) which can yield unintuitive results for None vs Some
or mismatched Some types; update gt and lt to explicitly handle option shapes:
add cases for (None, None), (None, Some(_)) and (Some(_), None) and a clear
policy for mismatched Some(x)/Some(y) (e.g., return false, raise an error, or
coerce/compare only when types match), keeping the existing BigDecimal branch
for (Some(BigDecimal...), Some(BigDecimal...)); reference the gt and lt
functions and the BigDecimal pattern to locate where to add these explicit
patterns.
🪄 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: 0b6468eb-8847-46c7-8501-11eb44f3563f
📒 Files selected for processing (8)
packages/envio/src/InMemoryTable.respackages/envio/src/LoadLayer.respackages/envio/src/LoadLayer.resipackages/envio/src/TableIndices.respackages/envio/src/TestIndexer.respackages/envio/src/UserContext.respackages/envio/src/db/EntityFilter.resscenarios/test_codegen/test/LoadLayer_test.res
💤 Files with no reviewable changes (1)
- packages/envio/src/TableIndices.res
getWhere now builds EntityFilter values directly (main's #1306); the filters carry API field names, which PgStorage resolves to the possibly renamed Postgres columns via Table.queryFields. https://claude.ai/code/session_01NDYWTmD2zFCoE7fqQqS7QK
2 participants
Summary
Consolidates the index/filter abstraction by moving
FieldValueand filter matching logic fromTableIndicesintoEntityFilter, and refactoring the in-memory index system to work directly withEntityFilter.tinstead of intermediateTableIndices.Index.tobjects.Key Changes
FieldValuemodule fromTableIndicestoEntityFilterwith all comparison operators (eq,gt,lt)TableIndicesmodule entirely (was only used for index representation)EntityFilter:toString: Generates stable cache keys for filtersmatches: Evaluates whether an entity matches a filter (replacesTableIndices.Index.evaluate)InMemoryTable.Entity:indicesByEntityId→filtersByEntityIdandfieldNameIndices→filterIndicesupdateIndicesanddeleteEntityFromIndicesto work directly withEntityFilter.tmakeIndicesSerializedToValuehelperLoadLayer.loadByField→LoadLayer.loadByFilter:EntityFilter.tdirectly instead of separatefieldName,operator,fieldValueparametersUserContext.getWhereHandler:EntityFilter.tvalues directly instead of usingTableIndices.Operator.tloadWithFilterhelper to reduce duplicationTestIndexer.handleLoad:EntityFilter.matchesEntityFilter.twith parsed field valuesImplementation Details
EntityFilter.toString, providing unambiguous cache keys for any filter configurationEntityFilter.FieldValue.t(anoptiontype) to handle missing/nullable fields gracefullyAndfilter recursively evaluates all nested filters, matching storage layer semanticsBigDecimal) are centralized inFieldValuemodule for consistencyhttps://claude.ai/code/session_01QN8cfbDEtGeoSGRxSXirpt
Summary by CodeRabbit