[ISSUE-830] Fix inverted embedding value filter that dropped digit-free text

[E2ern1ty]

Fixes #830.

What changes were proposed in this pull request?

The value filter feeding EmbeddingIndexStore kept values containing a digit or one of . _ - @ + ! $ % & = ~ and dropped everything else, so ordinary prose was never embedded while a bare id, a date or a run of punctuation was. SearchUtils.isAllAllowedChars returned on the first character inside the ignorable set rather than the first one outside it, which is the negation of both its name and its Javadoc, and SubgraphSemanticPromptFunction.verbalize(GraphEntity) keeps a value when that predicate is false.

Two changes, matching the split agreed in the issue.

Correct the predicate, and rename it to isAllIgnorableChars. It now returns true only when every character is ignorable, and treats null and empty as ignorable since callers use it to decide what to skip and there is nothing in them to index. The rename is deliberate rather than a fix in place: with the name unchanged, a caller depending on the previous meaning would silently flip behaviour, whereas a rename makes it fail to compile. Both call sites are in SubgraphSemanticPromptFunction and read the same as before, !isAllIgnorableChars(value). IGNORE_CHARS becomes IGNORABLE_CHARS, and the Javadoc now describes the set as characters that carry no meaning on their own.

Stop reporting entities with no embeddable text as indexed. This is the half that held regardless of the predicate's intended semantics, and it was requested on the issue. A digit-free entity logged Successfully added 1 new index items. Total indexed: 1 while producing nothing: the entity was registered in indexStoreMap with an empty vector list, so it could never be recalled, and nothing distinguished that from a successful build. indexBatch now warns with a count and an example key when entities yield no text, and the summary line reports how many entities actually hold vectors rather than how many were queued. From review, getEntityIndex also logs at debug level when an entity was checked and held nothing, which previously looked identical to an entity nobody had looked at.

How was this PR tested?

• Tests have Added for the changes
• Production environment verified

Unit tests. IgnorableTextFilterTest, 7 cases: the predicate over values that do and do not carry meaning, the property that whether a value is kept must not depend on it containing a digit, verbalization of digit-free vertex and edge text, verbalization dropping a value that is only a date, and the store either attempting a request or correctly declining to. All 7 fail against the previous behaviour.

Index lifecycle over successive runs. EmbeddingIndexLifecycleTest, added from review, runs three initStore calls against a local /v1/embeddings endpoint on an ephemeral port, so request and response serialization, the on-disk index file and the incremental decision are all real while the suite stays offline and deterministic. A meaningful vertex and edge are embedded and persisted while a date-only vertex is not, 2 lines; an unchanged graph on a second run issues no request and restores its vectors from the file; and when the date is replaced by meaningful text, that entity alone is embedded, the request body carries only its text, and the file grows to 3 lines. The last phase is the regression test for the review question of whether an entity that once yielded nothing is excluded for good: it is not, because initStore rebuilds indexStoreMap from the index file alone and nothing is persisted for such an entity.

Full geaflow-ai suite passes, 14 tests, 0 checkstyle violations.

End-to-end against a real embedding service. Both master (3f73eb55) and this branch were run through the full production path against SiliconFlow BAAI/bge-m3: real HTTPS requests, real 1024-dimension vectors, real on-disk index file, real recall through GraphMemoryServer. Corpus is the module's own text/Confucius, 532 chunks of Chinese prose, of which 532 contain no digit. Same harness source compiled against both trees, so the numbers are comparable.

	master	this branch
entities holding vectors	0 of 532	532 of 532
index file lines written	0	548
embedding requests issued	0	23, for 549 texts
summary log	Successfully added 532 new index items. Total indexed: 532	Successfully added 532 new index items. Entities holding vectors: 532 of 532
recall of a known chunk, embedding vector only	not possible, no vectors	returned, cosine 0.9999
recall for an unseen question, 学习和思考的关系是什么？	not possible	top chunk is 子曰：“学而时习之，不亦说乎？…”, cosine 0.5835
rebuild from the index file just written	n/a	0 new requests, 532 of 532 restored, recall still returns the chunk

So on master the whole corpus was reported as indexed while nothing was embedded and no request was ever issued. Recall was exercised with an embedding vector only, no keyword vector and no other index store registered, so anything returned had to come from the embedding index. The written index file holds 548 records of 1024 real dimensions each, 11.35 MB, keyed by entity, with no zero vectors.

The counterpart case on the same endpoint, 20 date-only values, which is what the old predicate selected for:

	master	this branch
entities holding vectors	20 of 20	0 of 20
embedding requests issued	1, for 20 inputs	0
log	Successfully added 20 new index items. Total indexed: 20	WARN 20 of 20 entities have no embeddable text and will hold no vectors, for example Vd1chunk, then Entities holding vectors: 0 of 20

The two tables together are the inversion the issue describes, measured against a live model rather than argued: master spends requests on dates and none on prose, this branch does the opposite and says so when there is nothing to embed.

Blast radius, measured before changing anything. On the LDBC test 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. The committed embedding index therefore needs no regeneration and GraphMemoryTest passes unchanged, which is also why the defect was invisible until now. It does mean the module's own end-to-end scenario was affected: MemoryServerTest imports those same 532 prose chunks, none of which would previously have been embedded.

Known gap, not addressed here. An entity already present in the index file whose value later changes is not re-embedded, because the index key is ModelUtils.getGraphEntityKey, id and label with no content in it, so the stale vector keeps matching. That predates this change and needs a content hash in the record plus an invalidation path; it deserves its own issue.

[kitalkuyo-gita]

First of all, thank you for your contribution. However, upon reviewing the repository, I noticed that there are still references to this function in some places. I have the following suggestions:

1. Perform a full-code search before merging: Search the entire apache/geaflow repository for all call sites of isAllAllowedChars to confirm that all instances have been updated in the PR. You should run a search like this:

grep -r "isAllAllowedChars" --include="*.java" .

2. Suggestion for a transition period regarding deprecated methods: Retain the old method but mark it as @Deprecated, and remove it only after 2–3 release cycles to avoid breaking downstream dependencies:

@Deprecated(since = "1.4.0", forRemoval = true)
public static boolean isAllAllowedChars(String str) {
return !isAllIgnorableChars(str);  // Delegate to the new method
}

[E2ern1ty]

Thanks. I ran exactly the search you asked for, and there are no remaining call sites.

$ grep -r "isAllAllowedChars" --include="*.java" .
./geaflow-ai/src/main/java/org/apache/geaflow/ai/operator/SearchUtils.java:97:
     * <p>This replaces {@code isAllAllowedChars}, whose loop returned on the first character

One hit, and it is the Javadoc line in this file explaining what the method replaces. Widening the search to every file type gives two more, both generated build output rather than source: target/site/jacoco/.../SearchUtils.java.html and target/apidocs/.../SearchUtils.html. Those are rebuilt from this Javadoc. If the references you saw were in a target/ directory or in the doc text, that is what they were. The two real call sites, both in SubgraphSemanticPromptFunction, are updated in this PR and now read !isAllIgnorableChars(value).

On the deprecated shim, I would rather not add it, for three reasons.

The delegation is not behaviour preserving, which is the part that worries me most. isAllAllowedChars returned false as soon as it met one ignorable character, whereas isAllIgnorableChars returns true only when every character is ignorable. Those are not complements. Take "a1": the old method returned false, because 1 is in the set, while !isAllIgnorableChars("a1") is !false, that is true. So return !isAllIgnorableChars(str) would silently flip the answer for any mixed value, and mixed values are the common case in real data. LDBC entity values are exactly letters plus digits. A shim that quietly changes behaviour for the majority of inputs is the hazard the rename was meant to remove, not a mitigation of it.

It also does not compile here. since and forRemoval were added to @Deprecated in Java 9, and this project builds at <jdk.version>1.8</jdk.version> (root pom.xml). And 1.4.0 is not a GeaFlow version; the tree is at 0.8.0-SNAPSHOT.

There is no downstream contract to protect either. SearchUtils is not present in any release tag, v0.8.0-rc1 included:

$ for t in v0.8.0-rc1 v0.7.0 v0.7.0-rc3 v0.7.0-rc2 v0.7.0-rc1; do
    printf "%s: " $t; git cat-file -e "$t:geaflow-ai/.../operator/SearchUtils.java" 2>/dev/null \
      && echo present || echo absent; done
v0.8.0-rc1: absent
v0.7.0: absent
...

It arrived with #716 in January, after the rc, so no published artifact has ever exposed this method and nothing outside the repo can be calling it. A deprecation window over two or three release cycles would keep a method whose semantics are the defect this PR fixes, and it would protect no one.

If you would still like a transition period, the only shim I would be comfortable adding is one that preserves the original contract rather than approximating it, return containsAnyIgnorableChar(str) under a name that says what it does. Happy to add that if you want it, but given that the class is unreleased I think deleting the old name is the better trade. Your call.

[kitalkuyo-gita]

Insufficient Exception Handling in EmbeddingIndexStore

Risk Description: In the indexBatch method, when an entity lacks embeddable text, a warning log is generated, but the entity is still registered in indexStoreMap (with an empty list of warnings). This can lead to the following issues:

During a second initialization, the containsKey check skips these entities (assuming they are already indexed).
Queries return empty results instead of a clear signal indicating "not indexed."

// Inside the indexBatch method - lines 210-223
for (Map.Entry<GraphEntity, Pair<Integer, Integer>> entry : entity2StartEndPair.entrySet()) {
GraphEntity e = entry.getKey();
List<EmbeddingService.EmbeddingResult> embeddings = new ArrayList<>();
for (int i = entry.getValue().getLeft(); i < entry.getValue().getRight(); i++) {
// If start == end (entities in withoutText), this loop does not execute
if (StringUtils.isNotBlank(result.get(i))) {
// ...
embeddings.add(res);
}
}
// ⚠️ Registered regardless of whether embeddings is empty
indexStoreMap.put(e, embeddings);  // embeddings might be an empty list
}

Problem scenario: Assume the graph contains two entities:

1st run: Entity A has text (embedding successful); Entity B has no text (empty list registered).
2nd run: Entity B's value is updated to meaningful text.
containsKey(B) returns true → skipped.
Entity B is never re-embedded.
Recommendation:

1. Do not register entities that yield no embeddings, or use a separate set for "checked but empty" entities:

// New field
private Set<GraphEntity> checkedButEmpty = new HashSet<>();

// Modified indexBatch
for (Map.Entry<GraphEntity, Pair<Integer, Integer>> entry : entity2StartEndPair.entrySet()) {
GraphEntity e = entry.getKey();
List<EmbeddingService.EmbeddingResult> embeddings = new ArrayList<>();
for (int i = entry.getValue().getLeft(); i < entry.getValue().getRight(); i++) {
if (StringUtils.isNotBlank(result.get(i))) {
EmbeddingService.EmbeddingResult res = gson.fromJson(result.get(i),
EmbeddingService.EmbeddingResult.class);
res.input = ModelUtils.getGraphEntityKey(e);
formatResult.add(gson.toJson(res)); 
embeddings.add(res);
}
}
if (!embeddings.isEmpty()) {
indexStoreMap.put(e, embeddings);
} else {
checkedButEmpty.add(e);  // Explicitly mark as checked but containing no content
}
}

// Modify the check logic in initStore
if (!indexStoreMap.containsKey(vertex) && !checkedButEmpty.contains(vertex)
&& !batchEntitiesBuffer.contains(vertex)) {
// Only entities that have truly not been processed enter the batch
batchEntitiesBuffer.add(vertex);
pendingEntities.add(vertex);
}

2. Enhance the distinction of return values ​​for getEntityIndex:

@Override
public List<IVector> getEntityIndex(GraphEntity entity) {
if (entity != null) {
List<EmbeddingService.EmbeddingResult> resultList = indexStoreMap.get(entity);
if (resultList != null) {
if (resultList.isEmpty()) {
LOGGER.debug("Entity {} was checked but contains no embeddable text", entity);
} else {
List<IVector> result = new ArrayList<>();
for (EmbeddingService.EmbeddingResult res : resultList) {
result.add(new EmbeddingVector(res.embedding));
}
return result;
}
}
}
return Collections.emptyList();
}

[E2ern1ty]

Good thing to check, and I went looking for it rather than reasoning about it. The skip you describe does not happen, and I have added a test that would fail if it did: EmbeddingIndexLifecycleTest, in b007b1a4.

The reason is in initStore rather than in indexBatch. Every call begins with

this.indexStoreMap = new HashMap<>();

and then repopulates the map from the index file only. Nothing is ever written to that file for an entity with no embeddable text, because indexBatch returns records for embedded texts alone. So on the next run the empty entry from the previous run does not exist, containsKey is false, and the entity is queued again. Your scenario ends the other way round: entity B does get re-embedded as soon as its value carries meaning.

The test runs three initStore calls against a local /v1/embeddings endpoint, so the request and response serialisation, the file and the decision about what to embed are all real, with no model service and no key. Run 1: a meaningful vertex and a meaningful edge are embedded and persisted, a date-only vertex is not, 2 lines in the file. Run 2, unchanged graph: 0 requests, vectors restored from the file, still 2 lines. Run 3, the date replaced by the master replied in the temple: exactly 1 request, its body contains the new text and does not contain the already-indexed text, the file grows to 3 lines, and getEntityIndex for that entity is no longer empty. The log from run 2 shows the mechanism directly, 1 of 1 entities have no embeddable text, the 1 being that entity queued again rather than skipped.

On not registering the empty list at all: I looked at that and it would cost something. MemoryGraph.scanEdge(vertex) returns both out-edges and in-edges, so initStore reaches the same edge from each of its endpoints. For an edge with no embeddable text, the empty entry in indexStoreMap is what makes the second visit a no-op. Dropping it without a replacement means verbalizing that edge twice per run, and the checkedButEmpty set restores the skip but also adds a second piece of state that has to be kept in step with the map and reset on every initStore, for a saving of one local verbalize call. Given that the cross-run recovery above depends on those entities not being remembered, I would rather keep one map whose meaning is "these are the vectors I have" than two structures that have to agree.

Your second suggestion I have taken, in a smaller form. getEntityIndex returned an empty list for two different situations, an entity that was checked and held nothing, and an entity nobody has looked at. It still returns the same value, since a caller wants vectors either way and has nothing useful to do with the distinction, but the first case now logs at debug level instead of being silent.

One nearby gap that is real and that I have deliberately not touched: an entity that is already in the index file and whose value then changes is not re-embedded, because the index key is ModelUtils.getGraphEntityKey, which is id and label with no content in it, so the stale vector keeps matching. That is independent of this fix, it predates it, and fixing it means a content hash in the record and an invalidation path. Happy to open a separate issue for it if you agree it is worth one.

[kitalkuyo-gita]

Although the newly added IgnorableTextFilterTest contains seven test cases, it lacks a crucial incremental indexing scenario:

Here is a simulated scenario:

// All tests in IgnorableTextFilterTest follow this pattern:
// 1. Build a new graph
// 2. Perform the initial initStore
//
// There are no tests covering:
// 1. Graph modification (an entity's text changes from "meaningless" to "meaningful")
// 2. Whether initStore correctly re-indexes upon a subsequent run
Missing test case:

Missing capability:

/**
* Missing test: Behavior when entity text changes from meaningless to meaningful
*/
@Test
public void testReindexWhenEntityValueChangesFromIgnorableToMeaningful(@TempDir Path tempDir)
throws Exception {
// First run: Entity A's value is "2024-01-01" (meaningless)
LocalMemoryGraphAccessor accessor1 = buildGraph("2024-01-01", "still none");
EmbeddingIndexStore store = new EmbeddingIndexStore();
store.initStore(accessor1, new SubgraphSemanticPromptFunction(accessor1),
tempDir.resolve("index1.jsonl").toString(),
new ModelConfig(null, null, null, null));

// Verify that no requests were sent during the first run
Assertions.assertTrue(Files.readAllLines(tempDir.resolve("index1.jsonl")).isEmpty());

// Second run: The value of the same entity is modified to meaningful text
//  Question: How do we simulate this scenario? Entity A already has a record in `indexStoreMap` (an empty vector list)
// `containsKey` returns true, causing reprocessing to be skipped
LocalMemoryGraphAccessor accessor2 = buildGraph("meaningful text", "still none");
// ... This scenario cannot be tested because the entity objects will differ
}

Suggestion:

Add a top-level integration test:

@Test
public void testCompleteIndexLifecycle(@TempDir Path tempDir) throws Exception {
// Phase 1: Initial indexing
LocalMemoryGraphAccessor graph1 = buildGraphWithMixedContent();
EmbeddingIndexStore store1 = new EmbeddingIndexStore();
store1.initStore(graph1, new SubgraphSemanticPromptFunction(graph1),
tempDir.resolve("index.jsonl").toString(), getRealModelConfig());
int firstRound = Files.readAllLines(tempDir.resolve("index.jsonl")).size();

// Phase 2: Delete some entities, add new entities
LocalMemoryGraphAccessor graph2 = buildGraphWithModifiedContent();
EmbeddingIndexStore store2 = new EmbeddingIndexStore();
store2.initStore(graph2, new SubgraphSemanticPromptFunction(graph2),
tempDir.resolve("index.jsonl").toString(), getRealModelConfig());
int secondRound = Files.readAllLines(tempDir.resolve("index.jsonl")).size();

// Verify the correctness of incremental indexing
Assertions.assertTrue(secondRound > firstRound || secondRound == firstRound);
}

[E2ern1ty]

Agreed, that gap was real. Added as EmbeddingIndexLifecycleTest in b007b1a4, and it covers the scenario your snippet was reaching for.

Two departures from the sketch. It does not need getRealModelConfig(): the test starts a local /v1/embeddings endpoint on an ephemeral port and returns deterministic vectors, so request and response serialisation, the on-disk index file and the incremental decision are all exercised for real, while the suite stays offline, needs no key and stays deterministic. And the assertion is exact rather than secondRound > firstRound || secondRound == firstRound, which holds for any outcome and so cannot fail: the test pins the line count to 2, then 2, then 3, and checks which text the request carried.

You noted in the comment that the change-of-value case looked untestable because the entity objects differ between the two graphs. That turns out not to be the obstacle, because the store does not match entities by object identity across runs. It writes and reads records keyed by ModelUtils.getGraphEntityKey, id and label, and initStore rebuilds its map from the file through key2EntityMap. So rebuilding the graph with the same ids and different text is exactly how the scenario is set up: buildGraph(MEANINGFUL, IGNORABLE) for the first two runs, then buildGraph(MEANINGFUL, "the master replied in the temple") for the third.

Phases, and what each pins down:

1. Mixed content. The meaningful vertex and the meaningful edge are embedded in one request and persisted, 2 lines; the date-only vertex holds no vector and contributes no line.
2. Same graph, new store, same file. 0 requests, vectors restored from the file, no duplicate records appended.
3. The date is replaced with meaningful text. Exactly 1 request, whose body contains the new text and not the text already in the file; the file grows to 3 lines; that entity now holds a vector.

Phase 3 is also the regression test for the concern in your other comment, that an entity yielding nothing once would be excluded for good. Full suite is now 14 tests, 0 checkstyle violations.

[E2ern1ty]

Thanks for the review. b007b1a4 responds to it; details are in the three inline replies, summary here.

Taken. The missing incremental scenario is now EmbeddingIndexLifecycleTest: three successive initStore calls against a local /v1/embeddings endpoint on an ephemeral port, so serialization, the on-disk file and the incremental decision are all real while the suite stays offline and deterministic. And getEntityIndex now says at debug level when an entity was checked and held nothing, which previously looked identical to an entity nobody had looked at.

Checked and not reproduced. The concern that an entity registered with an empty list would be skipped for good does not hold: initStore assigns a fresh indexStoreMap on every call and repopulates it from the index file alone, and nothing is persisted for an entity with no embeddable text. Phase 3 of the new test is the regression test for exactly your scenario, and the entity is embedded as soon as its value carries meaning. The run-2 log shows the mechanism, 1 of 1 entities have no embeddable text, the 1 being that entity queued again rather than skipped.

Pushed back on, with reasons. The grep -r "isAllAllowedChars" --include="*.java" . you asked for returns a single hit, the Javadoc line in SearchUtils explaining what the method replaces; the other matches are generated files under target/. There are no remaining call sites. I would rather not add the deprecated shim: return !isAllIgnorableChars(str) is not equivalent to the old method, since the old one returned false on the first ignorable character while the new one returns true only when all are, so "a1" gives false before and true after — a silent flip for mixed values, which is the common case. @Deprecated(since=, forRemoval=) also needs Java 9 and this project builds at 1.8, and SearchUtils is absent from every release tag including v0.8.0-rc1, so no published artifact exposes the method. If you still want a transition period I will add a shim that preserves the original contract exactly rather than approximating it — say the word.

Filed as separate, if you agree. An entity already in the index file whose value later changes is not re-embedded, because the index key carries no content. Pre-existing, independent of this fix, and needs a content hash plus invalidation.

Full suite is 14 tests, 0 checkstyle violations. The real-model end-to-end numbers in the description are unchanged by this commit.