Specialize entity index matching by field type; remove FieldValue#1343
Specialize entity index matching by field type; remove FieldValue#1343DZakh wants to merge 5 commits into
Conversation
Resolve each filter's comparison once from the table config when an index is registered, replacing the per-write operator switch and polymorphic Caml_obj comparison with native operators for primitive fields. Falls back to the structural FieldValue comparison for BigDecimal, Json, Date, arrays, and nullable ordering, where native ops would diverge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158A7T3rrBeU3xhaVEPwGUU
Resolve each indexed field's comparison once from the table's fieldType instead of routing every entity comparison through the polymorphic Caml_obj path. Collapse the generic matcher and the per-filter builder into a single makeMatcher, drop the FieldValue wrapper, and serialize cache keys with a standalone helper off the hot path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158A7T3rrBeU3xhaVEPwGUU
Add cases for nullish values on object-typed fields (no crash, no match), structural Json comparison, the empty-And throw, and toString cache-key serialization across every value type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158A7T3rrBeU3xhaVEPwGUU
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesEntityFilter matcher refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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
🤖 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 5-10: The `serializeValue` helper in `EntityFilter.res` is
producing colliding cache-key strings by mapping `null` and `undefined` to the
same value, using `toString()` for objects, and emitting raw strings inside
arrays. Update `serializeValue` so it encodes type information and escapes or
quotes string content deterministically for all supported values, including
nested arrays and plain objects, and make sure the new representation stays
stable for `getWhere` cache key generation.
🪄 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: 36046220-36bc-425d-9eb2-96a1358506ab
📒 Files selected for processing (6)
packages/envio/src/InMemoryTable.respackages/envio/src/LoadLayer.respackages/envio/src/TestIndexer.respackages/envio/src/db/EntityFilter.resscenarios/test_codegen/test/EntityFilter_test.resscenarios/test_codegen/test/helpers/MockIndexer.res
| let serializeValue: unknown => string = %raw(`function ser(v) { | ||
| if (v === undefined || v === null) return "undefined"; | ||
| if (Array.isArray(v)) return "[" + v.map(ser).join(",") + "]"; | ||
| if (typeof v === "object") return v.toString(); | ||
| return String(v); | ||
| }`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make serializeValue collision-free.
This serializer is now the source of truth for filter cache keys, but it conflates distinct values: null and undefined both become "undefined", plain JSON objects become "[object Object]", and strings/array elements are emitted without escaping. That means different getWhere filters can hash to the same key and reuse the wrong in-memory index or grouped load result.
🤖 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 5 - 10, The
`serializeValue` helper in `EntityFilter.res` is producing colliding cache-key
strings by mapping `null` and `undefined` to the same value, using `toString()`
for objects, and emitting raw strings inside arrays. Update `serializeValue` so
it encodes type information and escapes or quotes string content
deterministically for all supported values, including nested arrays and plain
objects, and make sure the new representation stays stable for `getWhere` cache
key generation.
Read indexed fields off the entity with a @get_index binding instead of casting it to a dict at every call site, so InMemoryTable, TestIndexer, and MockIndexer pass the entity through unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0158A7T3rrBeU3xhaVEPwGUU
2 participants
Why
In-memory entity indices (
getWhere/loadByField) matched every entity write against every registered filter throughEntityFilter.matches, which routed all comparisons throughFieldValue— anoption<@unboxed variant>wrapper whoseeq/gt/ltfell back to the polymorphicCaml_objpath for everything exceptBigDecimal. That comparison runs inupdateIndices, anO(entities × registered filters)loop on the hot write path, so the polymorphic dispatch and structural-compare overhead showed up per comparison.We already know each field's exact type from the table config, so the comparison can be resolved once per filter instead of re-dispatched per entity.
What
FieldValuemodule entirely.makeMatcher(~filter, ~table)replaces both the generic recursivematchesand the earlier per-filter builder. It resolves each field's comparison once fromfieldTypeinto a smallvalueComparerecord reused for every entity write. Entity dicts are nowdict<unknown>.===/>/<BigDecimal→ bignumber.js methodsDate→getTime()numeric compareJson→ structural deep compare (no native ordering exists)WHEREFieldValue.toStringto a standaloneserializeValuehelper, off the hot path (toStringis theLoadManagerhasher and has no table).matchescallers (InMemoryTableindex maintenance,TestIndexer,MockIndexer) tomakeMatcher.Behavior change (intentional)
Filtering a nullable field with
_gt/_ltwhere a row's value is null now yields no match (SQL-correct), instead of the previous undefined-ishCaml_objordering against null. This aligns in-memory matching with what Postgres returns for the same query.Tests
scenarios/test_codegen/test/EntityFilter_test.res:makeMatchertruth table — 17 filter cases × 3 rows covering every field kind, all operators, nullable-undefined, array equality + lexicographic ordering,Dateeq/gt, andAnd.BigDecimal/Date/ array fields → no crash, no match.Jsonfields compared structurally (distinct object, equal contents → match).Andthrows.toStringcache-key serialization across string/int/bigint/bool/bigdecimal/array/And.Verified:
pnpm rescriptclean;EntityFilter_test13/13;WriteRead_test(getWhere end-to-end through the index +MockIndexer) green.🤖 Generated with Claude Code
https://claude.ai/code/session_0158A7T3rrBeU3xhaVEPwGUU
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
New Features