Describe the bug
In geaflow-ai, the value filter that feeds EmbeddingIndexStore keeps values containing a digit or one of . _ - @ + ! $ % & = ~, and drops everything else. The practical effect is that ordinary prose is never embedded, while a bare id, a date or a run of punctuation is.
SearchUtils.isAllAllowedChars returns on the first character inside the character set rather than the first one outside it:
for (char c : str.toCharArray()) {
if (IGNORE_CHARS.contains(c)) {
return false;
}
}
return true;
and SubgraphSemanticPromptFunction.verbalize(GraphEntity), the overload EmbeddingIndexStore feeds on, keeps a value when the predicate is false:
.filter(str -> !SearchUtils.isAllAllowedChars(str))
Before opening this as a bug I want to ask whether it is intended, because I cannot rule out a deliberate "only vectorize identifier-like values" policy. My reading is that it is not intended, for four reasons that are independent of each other. If any of them is a misreading, please say so and I will close this.
- The name, the Javadoc and the call site all agree with the opposite behaviour; only the loop body disagrees. The method is named "is all allowed chars" and documented as
@return true if all characters are allowed. The set contains digits and symbols but no letters, so under the documented reading "abc" yields false and is kept, "12345" yields true and is dropped. That is the behaviour the negation at the call site asks for. One place disagreeing with three seems more likely than three places disagreeing with one.
- An empty value is sent to the embedding model. The predicate returns false for null and empty, and the call site keeps it. The early return even carries the comment
Consider empty/null invalid, so false was meant as "invalid", yet the caller keeps precisely the invalid ones.
- The keyword path applies no such filter. Keyword indexing goes through
verbalize(SubGraph), which renders everything. If dropping prose here were a division of labour between the two retrieval paths, the two would have to be complementary; instead one takes everything and the other only values containing a digit.
formatQuery runs immediately after the filter and strips much of what the filter selected on. EXCLUDED_CHARS and the ignorable set overlap on - and ., so a value kept because it contains - has that - replaced by a space in the next step.
Expected behavior
A value should be skipped when it carries no meaning on its own, such as a bare id or a date, and kept otherwise. Whether a value is embeddable should not depend on it containing a digit.
Additional context
Reproduction on master (6312f97d), no network and no API key needed. Two single vertex graphs whose text differs only by an appended digit, EmbeddingIndexStore.initStore with new ModelConfig(null, null, null, null):
| vertex text |
index lines written |
outcome |
no digits here at all |
0 |
completes, no request attempted |
has a digit 7 in it |
0 |
attempts the request, fails on the null url |
The digit-free case is silent in a second way: it logs Successfully added 1 new index items. Total indexed: 1 while having produced nothing. The entity is registered in indexStoreMap with an empty vector list, so it can never be recalled, and nothing distinguishes that from a successful build.
Why this has not been noticed. The only test covering embeddings, GraphMemoryTest#testLdbcMainPipeline, uses LDBC data whose values mix letters and digits. Both the current and the corrected predicate keep those, so the defect is invisible there. It also means the module's own end-to-end scenario is affected: MemoryServerTest imports 532 prose chunks, none of which would ever be embedded.
Proposed fix. I would split it in two, because only one half depends on the answer to the question above.
Unconditional, regardless of the intended semantics:
- Warn when an entity yields no embeddable text, with a count and an example key, and make the summary line report how many entities actually hold vectors rather than how many were queued. Silently reporting success for a no-op is a defect under any reading.
Conditional on the semantics being a bug:
- Correct the predicate to return true only when every character is ignorable, and treat null and empty as ignorable, since callers use it to decide what to skip and there is nothing in them to index.
- Rename it to
isAllIgnorableChars. Renaming rather than correcting in place is deliberate: with the name unchanged, any caller that depended on the previous meaning would silently flip behaviour, whereas a rename makes it fail to compile. Both call sites are in SubgraphSemanticPromptFunction and keep reading !isAllIgnorableChars(value).
Blast radius, measured before changing anything. On the LDBC dataset, of 168 entities the set wanting vectors is identical before and after the correction, and none of them are absent from the committed LDBCEmbeddingIndexStore file. The two predicates differ only on values that are entirely digits or punctuation, and on values that are entirely letters; LDBC entities have neither. So the committed embedding index needs no regeneration and GraphMemoryTest passes unchanged.
I have the change and tests ready locally, 7 cases covering the predicate, the property that appending a digit must not change whether a value is indexable, verbalization of digit-free vertex and edge text, and the store either attempting a request or correctly declining to. All 7 fail against the current behaviour. Happy to open a PR once the intent is confirmed, or to reduce it to the unconditional half if the filter is meant to work the way it does.
Describe the bug
In
geaflow-ai, the value filter that feedsEmbeddingIndexStorekeeps values containing a digit or one of. _ - @ + ! $ % & = ~, and drops everything else. The practical effect is that ordinary prose is never embedded, while a bare id, a date or a run of punctuation is.SearchUtils.isAllAllowedCharsreturns on the first character inside the character set rather than the first one outside it:and
SubgraphSemanticPromptFunction.verbalize(GraphEntity), the overloadEmbeddingIndexStorefeeds on, keeps a value when the predicate is false:Before opening this as a bug I want to ask whether it is intended, because I cannot rule out a deliberate "only vectorize identifier-like values" policy. My reading is that it is not intended, for four reasons that are independent of each other. If any of them is a misreading, please say so and I will close this.
@return true if all characters are allowed. The set contains digits and symbols but no letters, so under the documented reading"abc"yields false and is kept,"12345"yields true and is dropped. That is the behaviour the negation at the call site asks for. One place disagreeing with three seems more likely than three places disagreeing with one.Consider empty/null invalid, sofalsewas meant as "invalid", yet the caller keeps precisely the invalid ones.verbalize(SubGraph), which renders everything. If dropping prose here were a division of labour between the two retrieval paths, the two would have to be complementary; instead one takes everything and the other only values containing a digit.formatQueryruns immediately after the filter and strips much of what the filter selected on.EXCLUDED_CHARSand the ignorable set overlap on-and., so a value kept because it contains-has that-replaced by a space in the next step.Expected behavior
A value should be skipped when it carries no meaning on its own, such as a bare id or a date, and kept otherwise. Whether a value is embeddable should not depend on it containing a digit.
Additional context
Reproduction on
master(6312f97d), no network and no API key needed. Two single vertex graphs whose text differs only by an appended digit,EmbeddingIndexStore.initStorewithnew ModelConfig(null, null, null, null):no digits here at allhas a digit 7 in itThe digit-free case is silent in a second way: it logs
Successfully added 1 new index items. Total indexed: 1while having produced nothing. The entity is registered inindexStoreMapwith an empty vector list, so it can never be recalled, and nothing distinguishes that from a successful build.Why this has not been noticed. The only test covering embeddings,
GraphMemoryTest#testLdbcMainPipeline, uses LDBC data whose values mix letters and digits. Both the current and the corrected predicate keep those, so the defect is invisible there. It also means the module's own end-to-end scenario is affected:MemoryServerTestimports 532 prose chunks, none of which would ever be embedded.Proposed fix. I would split it in two, because only one half depends on the answer to the question above.
Unconditional, regardless of the intended semantics:
Conditional on the semantics being a bug:
isAllIgnorableChars. Renaming rather than correcting in place is deliberate: with the name unchanged, any caller that depended on the previous meaning would silently flip behaviour, whereas a rename makes it fail to compile. Both call sites are inSubgraphSemanticPromptFunctionand keep reading!isAllIgnorableChars(value).Blast radius, measured before changing anything. On the LDBC dataset, of 168 entities the set wanting vectors is identical before and after the correction, and none of them are absent from the committed
LDBCEmbeddingIndexStorefile. The two predicates differ only on values that are entirely digits or punctuation, and on values that are entirely letters; LDBC entities have neither. So the committed embedding index needs no regeneration andGraphMemoryTestpasses unchanged.I have the change and tests ready locally, 7 cases covering the predicate, the property that appending a digit must not change whether a value is indexable, verbalization of digit-free vertex and edge text, and the store either attempting a request or correctly declining to. All 7 fail against the current behaviour. Happy to open a PR once the intent is confirmed, or to reduce it to the unconditional half if the filter is meant to work the way it does.