Eng 1062 optimize all relations#558
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
maparent
changed the base branch from
main
to
eng-1012-update-query-builder-relation-conditions-to-use-reified
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
2 times, most recently
from
c3c3760 to
86e97e1
Compare
maparent
force-pushed
the
eng-1012-update-query-builder-relation-conditions-to-use-reified
branch
4 times, most recently
from
048ce21 to
e5f66c1
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
86e97e1 to
4ee948f
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
4ee948f to
c94af43
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
c94af43 to
c23d381
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
c23d381 to
4db181b
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
4db181b to
278e684
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
158b75d to
370ba81
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
370ba81 to
e3ca7ca
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
e3ca7ca to
7d1efe8
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
7d1efe8 to
374a2ce
Compare
maparent
force-pushed
the
ENG-1062-optimize-all-relations
branch
from
374a2ce to
926d8a8
Compare
| text: `node:${conditionUid}-Context`, | ||
| }); | ||
| } | ||
| if (ANY_RELATION_REGEX.test(r.label)) { |
There was a problem hiding this comment.
Detecting the "all relations" case
| }); | ||
|
|
||
| const relationsWithComplement = Array.from(uniqueRelations.values()); | ||
| const queryRelations = useReifiedRelations |
There was a problem hiding this comment.
Create a new case for all relations
| return "?relSchema"; | ||
| }, | ||
| mapper: () => { | ||
| // not sure here? |
There was a problem hiding this comment.
The results are the same between both versions, so I think I got it right, but I'll admit to a bit of guesswork here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
@maparent Devin flagged a few issues here. Could you take a look and address them, then re-tag me once they’re resolved? |
|
@mdroidian resolved the issues. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I like the high-level goal here: in reified-relations mode, discourse context should not have to run one query per relation type. A single “all relations touching this node” query, followed by regrouping, is the right direction.
I don’t think we should merge the current implementation shape as-is, though. It feels too brittle/indirect for a core discourse context path.
The main issue is that hasSchema and effectiveSource are being added as synthetic predefined selections so we can carry internal relation metadata out of the query. Predefined selections are part of the Query Builder/user-facing selection layer; these values are not real user selections, they are internal Datalog variables needed by this one caller. The placeholder mapper implementations returning "?relSchema" / "?relSource" are a signal that this is not really a selection abstraction. That also forces relSchema and relSource to be preserved globally in the relation translator.
There are also some correctness issues in the regrouping:
- The complement handling looks backwards. For keys ending in
-true, the code setsisComplement: truebut usesrelation.r.labelforlabel/text; for-false, it usesrelation.r.complement. Based on howuniqueRelationsis built, and based on the existingtext = isComplement ? r.complement : r.labellogic, those should be flipped. idbecomes the grouped key like${relationUid}-${direction}instead of the underlying relation schema id. That changes the meaning of the result id from relation id to implementation grouping key. Direction already hasisComplement, so the grouped key should not escape asid.onResultbehavior changed. PreviouslyexecuteQueriesreceivedonResultand could call it per relation query as each query completed; now results are buffered, empty groups are filtered, and callbacks fire at the end. That may be intentional, but it is a behavior change outside just the reified-relations optimization and should be tested/called out.
There are a few ways we could take this. I’ll elaborate on two:
- Option 1: add a
findVariablesinternal API. - Option 2: special-case the discourse context query outside of the Query Builder translator.
I put together an example of Option 1 here: #1160
That keeps the one-query approach and still reuses the existing Has Any Relation To translator, but it adds an internal findVariables API to fireQuery instead of adding fake predefined selections. So discourse context can explicitly ask for the scoped relSchema / relSource variables it needs, without exposing them as Query Builder selections.
Option 2 would be to special-case discourse context directly: since the discourse context overlay is the hot path here, we could build a direct reified-relation Datalog query for this flow and map the stored relation metadata into groups ourselves. That would be less reusable with Query Builder, but it may be the clearest implementation if this optimization is specifically for discourse context.
My recommendation is that we should not keep the fake predefined-selection approach. I’d be comfortable with either Option 1 from the example PR, or Option 2 as a dedicated discourse-context query path, but the current version is coupling too many layers in a brittle way.
|
Ok. I did not read the PR yet, but I like option 1 as a direction and would be happy to take over from your PR. (I would start later today or even tomorrow morning.) If you feel you started it and want to finish it, please let me know. |
|
Ok. Status: I corrected the nits. |
|
@mdroidian pushed Changes made:
Verified locally:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughAdds a ChangesReified Relations Query Mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/roam/src/utils/__tests__/fireQuery.test.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [ apps/roam/src/utils/__tests__/getDiscourseContextResults.test.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [ apps/roam/src/utils/deriveDiscourseNodeAttribute.tsOops! Something went wrong! :( ESLint: 8.57.1 YAMLException: Cannot read config file: /apps/roam/eslint.config.mjs 5 | export default [
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@apps/roam/src/utils/getDiscourseContextResults.ts`:
- Around line 281-314: The regrouping in getDiscourseContextResults is losing
the original schema order because Object.entries(byRel) iterates by insertion
order from raw rows instead of the existing relationsWithComplement order. Fix
the reified-path rebuild so it preserves the same ordering contract as the
pre-reified path by iterating in schema/relation order (using the existing
uniqueRelations/relationsWithComplement references) and only pulling matching
grouped results from byRel when constructing resultsWithRelation.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ab5aa5a-23c9-4f86-ba3b-b48980658afc
📒 Files selected for processing (7)
apps/roam/src/utils/__tests__/fireQuery.test.tsapps/roam/src/utils/__tests__/getDiscourseContextResults.test.tsapps/roam/src/utils/deriveDiscourseNodeAttribute.tsapps/roam/src/utils/fireQuery.tsapps/roam/src/utils/getDiscourseContextResults.tsapps/roam/src/utils/predefinedSelections.tsapps/roam/src/utils/registerDiscourseDatalogTranslators.ts
2 participants
Use a pseudo-relation for a query for all relations. Rework the algorithm for grouping results, which means adding some selections.
https://www.loom.com/share/ced1e4cf67ec4acdb97db690431ee9a0
Summary by CodeRabbit
New Features
Bug Fixes
Tests