You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
QueryDocument.getEmbeddingInput() reads the obs-group concept name inline from the metadata map, while #60 added a centralizing accessor for the same field. They use different normalization, so the read the #60 accessor was meant to centralize still has a second, divergent implementation inside the model itself. Converge them — but it is a deliberate decision, not a trivial refactor, because converging changes embedding behavior (see below).
getEmbeddingInput() (QueryDocument.java, the group-prefix branch) reads the field directly:
ObjectgroupName = metadata.get(FIELD_OBS_GROUP_CONCEPT_NAME);
if (groupNameinstanceofString && !((String) groupName).isEmpty()) {
sb.append(groupName).append(" — "); // raw value, no trim; skip-empty (not skip-blank)
}
i.e. raw value + skip-empty, no trim.
Failure mode this prevents (silent divergence)
Low probability, but the same drift class #60 addressed — only internal: if querystore later changes how obs_group_concept_name is normalized, the canonical getObsGroupConceptName() accessor updates in one place, but getEmbeddingInput()'s inline raw read does not, so the consumer-facing value and the embedding-prefix could drift for the same field.
Why it was deferred (the real decision)
This is not a free refactor — getEmbeddingInput() feeds the embedding vectors, so changing its read changes the vectors:
Option A — make getEmbeddingInput() use the accessor (trim + blank→null). Cleaner, single read shape, and arguably more correct (a whitespace-only group name currently emits a " — " prefix; trimming would drop it). But it changes the embedding-input text for any record whose group name has surrounding/!empty-but-blank whitespace → different vectors → requires re-running the retrieval eval rubric (RetrievalQualityEvalTest + the MySQL/ES integration variants) to confirm no regression. In practice group names come from ConceptNameUtil.getPreferredName and are rarely padded, so the real-data impact is likely nil — but it must be verified, not assumed.
Option B — give the accessor getEmbeddingInput()'s raw/no-trim semantics. Avoids any embedding change and lets getEmbeddingInput() reuse it cleanly, but contradicts Add typed getObsGroupUuid() / getObsGroupConceptName() accessors on QueryDocument #60's "clean String" intent for the consumer-facing accessor (consumers would get untrimmed values).
A decision between A and B is needed before converging.
Scope / acceptance
Decide A vs B (recommend A — "clean String" everywhere — if the eval confirms no regression).
Converge getEmbeddingInput()'s group-name read onto the chosen shape so the field is read one way in the model.
If Option A: re-run the eval rubric and record that recall@30 is unchanged (still ≥ the 0.40 gate / ~0.888 baseline) in the PR.
Summary
QueryDocument.getEmbeddingInput()reads the obs-group concept name inline from the metadata map, while #60 added a centralizing accessor for the same field. They use different normalization, so the read the #60 accessor was meant to centralize still has a second, divergent implementation inside the model itself. Converge them — but it is a deliberate decision, not a trivial refactor, because converging changes embedding behavior (see below).Current state
getObsGroupConceptName()(added in Add typed getObsGroupUuid() / getObsGroupConceptName() accessors on QueryDocument #60) →metadataString(FIELD_OBS_GROUP_CONCEPT_NAME): trim + blank→null("clean String", identity semantics).getEmbeddingInput()(QueryDocument.java, the group-prefix branch) reads the field directly:Failure mode this prevents (silent divergence)
Low probability, but the same drift class #60 addressed — only internal: if querystore later changes how
obs_group_concept_nameis normalized, the canonicalgetObsGroupConceptName()accessor updates in one place, butgetEmbeddingInput()'s inline raw read does not, so the consumer-facing value and the embedding-prefix could drift for the same field.Why it was deferred (the real decision)
This is not a free refactor —
getEmbeddingInput()feeds the embedding vectors, so changing its read changes the vectors:getEmbeddingInput()use the accessor (trim + blank→null). Cleaner, single read shape, and arguably more correct (a whitespace-only group name currently emits a" — "prefix; trimming would drop it). But it changes the embedding-input text for any record whose group name has surrounding/!empty-but-blank whitespace → different vectors → requires re-running the retrieval eval rubric (RetrievalQualityEvalTest+ the MySQL/ES integration variants) to confirm no regression. In practice group names come fromConceptNameUtil.getPreferredNameand are rarely padded, so the real-data impact is likely nil — but it must be verified, not assumed.getEmbeddingInput()'s raw/no-trim semantics. Avoids any embedding change and letsgetEmbeddingInput()reuse it cleanly, but contradicts Add typed getObsGroupUuid() / getObsGroupConceptName() accessors on QueryDocument #60's "clean String" intent for the consumer-facing accessor (consumers would get untrimmed values).A decision between A and B is needed before converging.
Scope / acceptance
getEmbeddingInput()'s group-name read onto the chosen shape so the field is read one way in the model.Notes
getEmbeddingInput()is correct and tested today; this is a reuse/consistency tidy, like Add typed getObsGroupUuid() / getObsGroupConceptName() accessors on QueryDocument #60 itself.