Conversation
|
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
left a comment
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
| Matcher matcher = WORD_PATTERN.matcher(text); | |
| Matcher matcher = WORD_PATTERN.matcher(Normalizer.normalize(text, Normalizer.Form.NFC)); |
(plus import java.text.Normalizer;)
There was a problem hiding this comment.
Fixed in a5ee075, with a test on decomposed cafe\u0301.
| } | ||
|
|
||
| @Test | ||
| public void searchMemory_nonAsciiWord_matchesWholeWord() { |
There was a problem hiding this comment.
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 서울?
There was a problem hiding this comment.
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+. |
There was a problem hiding this comment.
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.
| // Pattern to extract words, matching Python's Unicode-aware \w+. | |
| // Unicode-aware word pattern, close to Python's \w+. |
1b34bd9 to
a5ee075
Compare
dosadczuk
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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.
a5ee075 to
ce84b4b
Compare
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Aligns
InMemoryMemoryService.searchMemorywith the word extraction of the adk-pythonInMemoryMemoryService, 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:weather?does not match an event withweather in Seoul(the query word isweather?).cafédoes not matchMeet me at the café(the event word iscaf).12345does not matchOrder 12345 shipped(digits are dropped from the event).The comment on the pattern says it matches the Python version.
Solution:
As in adk-python:
\w+andPattern.UNICODE_CHARACTER_CLASS, close to Python's Unicode\w+; it is used for both the query and the event text._extract_searchable_wordsdoes, sopythonstill matches私はPythonを使うwhilethondoes not. As in Python, a character is Latin if it is an ASCII letter, digit or_, or if its Unicode name starts withLATIN, sonº12345contributes12345.query_word in event_text_lowerdoes: Japanese and Chinese put no spaces between words, so太郎matches私の名前は太郎です.Words now include digits and underscores: a query for
foono longer matchesfoo123, andcaseno longer matchessnake_case. adk-python behaves the same way.Testing Plan
Unit Tests:
Tests in
InMemoryMemoryServiceTestcover the three cases above (withcafnot matchingcafé), 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, andnº12345matching12345. 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