Skip to content

fix(memory): tokenize query and events alike in InMemoryMemoryService - #1573

Open
Laurianti wants to merge 1 commit into
google:mainfrom
Laurianti:fix-memory-search-word-matching
Open

Laurianti wants to merge 1 commit into
google:mainfrom
Laurianti:fix-memory-search-word-matching

Conversation

@Laurianti

@Laurianti Laurianti commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Link to Issue or Description of Change

2. Or, if no issue exists, describe the change:

Aligns InMemoryMemoryService.searchMemory with the word extraction of the adk-python InMemoryMemoryService, which uses \w+ for both the query and the event text.

Problem:

The Java service splits the query on whitespace but extracts event words with [A-Za-z]+, so the two sides do not produce the same words:

  • query weather? does not match an event with weather in Seoul (the query word is weather?).
  • query café does not match Meet me at the café (the event word is caf).
  • query 12345 does not match Order 12345 shipped (digits are dropped from the event).

The comment on the pattern says it matches the Python version.

Solution:

As in adk-python:

  • One helper normalizes the text to NFC and extracts lowercase words with \w+ and Pattern.UNICODE_CHARACTER_CLASS, close to Python's Unicode \w+; it is used for both the query and the event text.
  • For event text, a word that mixes Latin and non-Latin characters also contributes each single-script run, as _extract_searchable_words does, so python still matches 私はPythonを使う while thon does not. As in Python, a character is Latin if it is an ASCII letter, digit or _, or if its Unicode name starts with LATIN, so nº12345 contributes 12345.
  • A query word that is not ASCII also matches anywhere in the event text, NFC-normalized and lowercased, as query_word in event_text_lower does: Japanese and Chinese put no spaces between words, so 太郎 matches 私の名前は太郎です.

Words now include digits and underscores: a query for foo no longer matches foo123, and case no longer matches snake_case. adk-python behaves the same way.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Tests in InMemoryMemoryServiceTest cover the three cases above (with caf not matching café), decomposed text matching a precomposed query, a Latin word inside Japanese text, the adk-python cases for a non-ASCII query word inside Japanese and Chinese text, and nº12345 matching 12345. Each fails when the matching part of the change is removed. mvn -pl core test: 1890 tests, 0 failures, 0 errors, 24 skipped.

Manual End-to-End (E2E) Tests:

Not needed: the change is limited to word extraction in the in-memory service.

Checklist

  • I have read the CONTRIBUTING.md document.
  • My pull request contains a single commit.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.

@hemasekhar-p hemasekhar-p self-assigned this Sep 29, 2026
@hemasekhar-p

Copy link
Copy Markdown
Contributor

Hi @Laurianti, We appreciate your contribution and the effort you put into this pull request. It is currently under review by our team. We will update you if any additional details are needed. Thank you.

@dosadczuk dosadczuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. The query and event mismatch is real, and the three new tests cover it well. I am requesting changes on one regression, the missing NFC normalization, and the gap in the café test.

Please also note in the PR description that tokens now include digits and underscores. A query for foo will no longer match foo123, and case will not match snake_case. adk-python works this way, so the change is intended.

wordsInEvent.add(matcher.group().toLowerCase(Locale.ROOT));
}
}
wordsInEvent.addAll(extractWordsLower(text));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks matching for Latin words embedded in Japanese or Chinese text. Today [A-Za-z]+ pulls python out of 私はPythonを使う, so the query python matches. With Unicode \w+, the entire string turns into a single token, and the query no longer matches.

Python handles this in _extract_searchable_words, splitting whenever the text switches between Latin and non-Latin characters. Could you port that logic for event text and add a test? For Latin, you can treat ASCII letters, digits, and _ as Latin, and check Character.UnicodeScript.of(cp) == Character.UnicodeScript.LATIN for anything above ASCII

@Laurianti Laurianti Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a5ee075: ported _extract_searchable_words for event text, with ASCII letters, digits and _ or UnicodeScript.LATIN as Latin; added a test with 私はPythonでADKを使っています (python and adk match, thon and java do not).

/** Extracts words from a string and converts them to lowercase. */
private static ImmutableSet<String> extractWordsLower(String text) {
ImmutableSet.Builder<String> words = ImmutableSet.builder();
Matcher matcher = WORD_PATTERN.matcher(text);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python's _extract_words_lower runs NFC normalization before tokenizing. Without that, decomposed input like cafe\u0301 fails to match precomposed café, because \w with UNICODE_CHARACTER_CLASS treats the accent mark as part of the word.

Suggested change
Matcher matcher = WORD_PATTERN.matcher(text);
Matcher matcher = WORD_PATTERN.matcher(Normalizer.normalize(text, Normalizer.Form.NFC));

(plus import java.text.Normalizer;)

@Laurianti Laurianti Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a5ee075, with a test on decomposed cafe\u0301.

}

@Test
public void searchMemory_nonAsciiWord_matchesWholeWord() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes even without Pattern.UNICODE_CHARACTER_CLASS. Plain \w+ cuts both café and the query down to caf, so they still match by accident.

Could you assert that querying caf returns nothing (matching the matchesWholeWord contract), or switch to a non-ASCII word like 서울?

@Laurianti Laurianti Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a5ee075: the test now also asserts that caf returns nothing.


// Pattern to extract words, matching the Python version.
private static final Pattern WORD_PATTERN = Pattern.compile("[A-Za-z]+");
// Pattern to extract words, matching Python's Unicode-aware \w+.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java's \w with UNICODE_CHARACTER_CLASS differs slightly from Python's. It treats combining marks as word characters (हिन्दी stays whole rather than splitting) and skips digits like ².

Java's behavior is actually better here, so we can keep the regex. Just update the comment so we do not claim it matches Python exactly.

Suggested change
// Pattern to extract words, matching Python's Unicode-aware \w+.
// Unicode-aware word pattern, close to Python's \w+.

@Laurianti Laurianti Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a5ee075.

@dosadczuk dosadczuk added waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. needs update and removed needs review labels Sep 30, 2026
@Laurianti
Laurianti force-pushed the fix-memory-search-word-matching branch from 1b34bd9 to a5ee075 Compare September 30, 2026 10:52

@dosadczuk dosadczuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. Everything from the last round is addressed. I have one more parity point below, the substring match Python does for non-ASCII queries, plus one small thing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python has one more rule here: a non-ASCII query word also matches if it appears anywhere in the event text (query_word in event_text_lower). Without it, 太郎 doesn't find 私の名前は太郎です, and 使って doesn't find the text in your new test. Python's tests expect both to match. Could you add that rule and port those test cases? A follow-up PR is fine too if you'd rather keep this one smaller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ce84b4b: a non-ASCII query word also matches anywhere in the event text, NFC-normalized and lowercased, and searchMemory_nonAsciiQueryWord_matchesInsideUnspacedText ports the adk-python cases: 太郎 and 使って match, 天気 and 天气预报 do not, 机器学习 matches.

if (codePoint < 0x80) {
return Character.isLetterOrDigit(codePoint) || codePoint == '_';
}
return Character.UnicodeScript.of(codePoint) == Character.UnicodeScript.LATIN;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's on me. UnicodeScript.LATIN isn't quite what Python checks: Python tests whether the character's name starts with LATIN. The two differ for characters like º and ª, so Pedido nº12345 matches 12345 in Python but not here. Checking Character.getName(codePoint) for the LATIN prefix would match Python exactly (it can return null).

@Laurianti Laurianti Oct 1, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ce84b4b: isLatin now checks that Character.getName(codePoint) is not null and starts with LATIN, and searchMemory_ordinalIndicatorBeforeDigits_matchesDigits checks that 12345 matches Pedido nº12345.

@Laurianti
Laurianti force-pushed the fix-memory-search-word-matching branch from a5ee075 to ce84b4b Compare October 1, 2026 12:10
@Laurianti
Laurianti requested a review from dosadczuk October 2, 2026 04:19
@hemasekhar-p hemasekhar-p added needs review and removed waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. labels Oct 19, 2026

@dosadczuk dosadczuk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, and sorry for one more round. Both points from the last round are fixed, but two problems below come from my own earlier advice: the word pattern, and a gap in the NFC tests. I also ran the adk-python search tests against this change, and all of their tokenizer cases pass.

Comment on lines +48 to +50
// Unicode-aware word pattern, close to Python's \w+.
private static final Pattern WORD_PATTERN =
Pattern.compile("\\w+", Pattern.UNICODE_CHARACTER_CLASS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my earlier note to keep this pattern was wrong, and it costs you another round. The pattern has two problems:

  1. Android's Pattern rejects this flag.
  2. With this flag, \w also matches combining marks, joiners and connector punctuation, and it does not match numbers such as ². The emoji variation selector U+FE0F and the zero-width joiner U+200D become words, so the query thanks ❤️ matches the event ⚠️ disk full. adk-python returns no match.
    [\p{L}\p{N}_] matches the same characters as Python's \w, and it needs no flag. Like adk-python, it splits a word at a combining mark, for example in Hindi.
Suggested change
// Unicode-aware word pattern, close to Python's \w+.
private static final Pattern WORD_PATTERN =
Pattern.compile("\\w+", Pattern.UNICODE_CHARACTER_CLASS);
// Letters, numbers and _, as Python's \w+ matches them.
private static final Pattern WORD_PATTERN = Pattern.compile("[\\p{L}\\p{N}_]+");

@Laurianti Laurianti Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 008d409: [\p{L}\p{N}_]+, as suggested. Added searchMemory_emojiVariationSelectorAndJoiner_areNotWords (your thanks ❤️ case, and one with U+200D) and searchMemory_superscriptDigit_isPartOfWord (x²); both fail with the previous pattern.

@@ -103,21 +104,32 @@ public Single<SearchMemoryResponse> searchMemory(String appName, String userId,
}

Set<String> wordsInEvent = new HashSet<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Join the part texts first and extract the words once, as adk-python does. Then you do not need the HashSet, and you can remove the HashSet and Set imports

@Laurianti Laurianti Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 008d409: the part texts are joined first and the words extracted once, so the HashSet and Set imports are gone.

@dosadczuk dosadczuk added waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale. and removed needs review labels Oct 20, 2026
@Laurianti
Laurianti force-pushed the fix-memory-search-word-matching branch from ce84b4b to ce8f38f Compare October 20, 2026 15:33
@Laurianti

Copy link
Copy Markdown
Contributor Author

Added searchMemory_decomposedQuery_matchesPrecomposedText, the other direction of the existing NFC test: it fails if the query is not normalized.

Matches adk-python: both sides are normalized to NFC and split with a
Unicode-aware \w+, so a query word followed by punctuation, a word with
accented letters and a number now match. For event text, a word that mixes
Latin and non-Latin characters also yields each single-script run, so a
Latin word inside Japanese or Chinese text still matches. Before, the query
was split on whitespace only and events kept only ASCII letters.
@Laurianti
Laurianti force-pushed the fix-memory-search-word-matching branch from ce8f38f to 008d409 Compare October 20, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs update waiting on reporter Waiting for reaction by reporter. Failing that, maintainers will eventually closed it as stale.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants