ENG-463 autogenerate arity column #217
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change updates the "Concept" table by redefining the "arity" column as a generated stored column, computed dynamically using new SQL functions that count elements in a JSONB "roles" array. It also updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant compute_arity_local
participant compute_arity_id
participant compute_arity_lit
Client->>Database: Insert/Update Concept row
Database->>compute_arity_local: compute_arity_local(schema_id, literal_content)
alt schema_id is not null
compute_arity_local->>compute_arity_id: compute_arity_id(schema_id)
compute_arity_id-->>compute_arity_local: arity count
else schema_id is null
compute_arity_local->>compute_arity_lit: compute_arity_lit(literal_content)
compute_arity_lit-->>compute_arity_local: arity count
end
compute_arity_local-->>Database: arity value
Database-->>Client: Row with computed arity
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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 (7)
packages/database/supabase/schemas/concept.sql (4)
16-17: Ensure robust JSON parsing and uniqueness in extract_references.The use of
jsonb_path_query_arraywithDISTINCThandles duplicates, but you may want to guard against unexpected JSON structures or non-numeric values causing cast errors. Consider validating withjsonb_typeofbefore casting or falling back gracefully when invalid.
19-21: Optimize arity calculation withjsonb_array_length.Extracting each element into a CTE and counting can be simplified and accelerated using:
SELECT COALESCE(jsonb_array_length(lit_content->'roles'), 0);This reduces overhead and improves readability.
23-27: Avoid per-row subquery overhead incompute_arity_id.Querying
public."Concept"inside this function for every invocation could become a performance bottleneck on large tables. Consider passing pre-fetched JSONB or computing role counts via triggers/materialized views to eliminate the on-the-fly lookup.
29-31: Add null/empty handling forcompute_arity_local.If
literal_contentis ever null or missing theroleskey, the function may error. Wrapping withCOALESCE(lit_content, '{}'::jsonb)and coalescing the result to zero will prevent unexpected failures.packages/database/supabase/migrations/20250616212706_generate-arity.sql (3)
5-11: Refactorcompute_arity_idfor clarity and performance.Instead of iterating with
jsonb_path_query, you can use:SELECT COALESCE(jsonb_array_length(literal_content->'roles'), 0) FROM public."Concept" WHERE id = p_schema_id;This streamlines the function and reduces CTE overhead.
13-19: Simplifycompute_arity_litusing direct array length.Replace the CTE approach with:
SELECT COALESCE(jsonb_array_length(lit_content->'roles'), 0);This keeps immutability and lowers execution cost.
21-27: Hardencompute_arity_localwith defensive defaults.Wrap
lit_contentinCOALESCE(lit_content, '{}'::jsonb)and handle missing schema rows explicitly (e.g., return 0 if no concept found) to avoid null or lookup errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/database/supabase/migrations/20250616212706_generate-arity.sql(1 hunks)packages/database/supabase/schemas/concept.sql(2 hunks)
🔇 Additional comments (3)
packages/database/supabase/schemas/concept.sql (2)
45-45: Verify backfill of generatedaritycolumn.Dropping and re-adding this column will backfill based on existing data—add a sanity query or test post-migration to confirm there are no nulls or miscounts.
49-49: Generatedrefscolumn implementation looks solid.Using
DISTINCTandCOALESCEensures duplicate elimination and empty-array handling, and the GIN index onrefsshould preserve query performance.packages/database/supabase/migrations/20250616212706_generate-arity.sql (1)
29-35:extract_referencesdefinition matches schema file.The migration’s
CREATE OR REPLACE FUNCTIONis consistent with the schema and indexing strategy; no changes needed here.
packages/database/supabase/migrations/20250616212706_generate-arity.sql
Outdated
Show resolved
Hide resolved
|
I'm curious why this is merging into ENG-446-bulk-db-concept-upsert instead of |
|
Actually, good point; it was because the change was prompted by a coderabbit comment on 446. But no reason not to rebase on main. |
maparent
force-pushed
the
ENG-463-autogenerate-arity
branch
from
54109b2 to
3a7d7ed
Compare
Summary by CodeRabbit