fix: batch domain-label inserts under one stable query per label - #499
fix: batch domain-label inserts under one stable query per label#499yzxcj797 wants to merge 1 commit into
Conversation
insert_domain_entity built its Cypher with a fresh new_query_var() UUID
and an // awsqid:{e_id}-{e_label} comment interpolated into the query
text. GraphBatchClient groups param rows by the full query string, so
every domain-label insert produced a unique single-row batch: batching
and dedup were defeated, self.batches grew one full query string per
fact-entity occurrence, and each insert paid its own round trip.
Keep the query text stable per label: a fixed variable (the single
MERGE needs no collision avoidance), and a label-scoped comment. The
entity id already travels in the params row.
Fixes awslabs#477
|
Thank you @yzxcj797 for your contribution we will review it and respond shortly. |
noel-improv
left a comment
noel-improv
left a comment
There was a problem hiding this comment.
thanks for your contribution @yzxcj797, please review these requested changes.
| # batching and dedup entirely (#477). The entity id already | ||
| # travels in the params; the comment stays label-scoped. | ||
| e_comment = f'// awsqid:domain-label-{e_label}'.replace('\r', ' ').replace('\n', ' ') | ||
| query_e = f"UNWIND $params AS params MERGE (e:`__Entity__`{{{graph_client.node_id('entityId')}: params.entityId}}) SET e :`{e_label}` {e_comment}" |
There was a problem hiding this comment.
Confirmed the differential locally rather than reading it off the diff. Against current main's builder the new test fails with three separate query keys; on this branch both pass:
AssertionError: expected one shared domain-label query, got 3
Build test dir is 27 failed / 15 errors on both this branch and origin/main, so nothing here regressed.
escape_cypher_label and the \r/\n stripping both survive, and pulling e_id out of the query text leaves less inlined than before. No objection from the injection-hardening side.
| # text makes every insert its own single-row batch and defeats | ||
| # batching and dedup entirely (#477). The entity id already | ||
| # travels in the params; the comment stays label-scoped. | ||
| e_comment = f'// awsqid:domain-label-{e_label}'.replace('\r', ' ').replace('\n', ' ') |
There was a problem hiding this comment.
Dropping the per-entity id is safe. The only consumer of awsqid is _add_parameterless_query (graph_batch_client.py:111), reached only when properties is falsy — and since #476 this query always passes _to_params(...), so it was already dead on this path.
| return | ||
|
|
||
| e_var = new_query_var() | ||
| e_id = entity.entityId |
There was a problem hiding this comment.
Nit: new_query_var is now unused in this file (import on line 9). Worth dropping with it.
There was a problem hiding this comment.
Rebase note: this predates dc6a096f (downmerge #502), which already landed #476's UNWIND $params / _to_params conversion on main, hence the conflict. After a rebase the change reduces to the fixed e var plus the label-scoped comment. Main also carries a comment above this block calling the batching "a future optimization"; that should go with the fix.
3 participants
Fixes #477.
Root cause
EntityGraphBuilder.insert_domain_entitybuilds its Cypher with a freshnew_query_var()UUID and an// awsqid:{e_id}-{e_label}comment interpolated directly into the query text.GraphBatchClient.execute_query_with_retrygroups param rows by the full query string (self.batches[query].extend(...)), so both the per-call variable name and the per-entity id made every domain-label insert a unique batch key holding a single param row — batching and dedup defeated,self.batchesgrowing one full query string per fact-entity occurrence, one round trip per insert.Fix
The query text is now stable per label:
evariable — the statement has exactly one MERGE, so the UUID collision-avoidance the helper provides isn't needed here;// awsqid:domain-label-{label}) instead of the per-entity id — the id already travels in the params row. The comment keeps its newline/carriage-return stripping and the label stays escaped viaescape_cypher_label, preserving the injection hardening from the earlier fix this test file covers.Inserts for distinct entities of the same classification now share one batch entry (
UNWIND $paramsMERGEs them in one statement), and different classifications group under their own query.Testing
Added
test_domain_entity_inserts_batch_under_one_query_per_labelnext to the existing regression tests for this builder: three facts with distinct entity ids of the same classification must queue under exactly one domain query with all three id rows, instead of three single-row batches.