Skip to content

add CompletionFilter - #249

Open
katsuta-a-wap wants to merge 2 commits into
developfrom
feature/completion-filter-develop
Open

add CompletionFilter#249
katsuta-a-wap wants to merge 2 commits into
developfrom
feature/completion-filter-develop

Conversation

@katsuta-a-wap

Copy link
Copy Markdown

target branch: develop
backport: all

Implement SudachiCompletionFilter.
The search and test-scripts will be separated into es and os branch.

#155 is supported.

@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

while (!tokenGenerator.hasNext()) {
if (!inputStreamConsumed && input.incrementToken()) {
val m: Morpheme = morphemeAtt.getMorpheme()
val surface: String = m.surface()

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.

I think surface should be taken from termAttr, in case of e.g. normalized_form filter is applied beforehand (we may also need to note that in this case reading and surface may not match due to stemming).

override fun incrementToken(): Boolean {
while (!tokenGenerator.hasNext()) {
if (!inputStreamConsumed && input.incrementToken()) {
val m: Morpheme = morphemeAtt.getMorpheme()

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.

morphemeAttr.getMorpheme may return null and we need to handle that case.

when {
!isAllLowercaseAlphabet(pdgSurface.toString()) &&
isAllLowercaseAlphabet(surface.toString()) -> {
pdgSurface.append(surface)

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.


if (tokenGenerator.hasNext()) {
clearAttributes()
val token: CompletionToken = tokenGenerator.next()

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.

We also need to set morpheme attribute here.
I wonder how we should do when we concatenate tokens during query mode... Probably we can omit morpheme attr (set null) in that case, but I'm not fully sure if it's safe.


private fun romanize(text: String): List<String> {
val output = mutableListOf<String>()
val romaji: String = convertFullWidthAlphabetToHalfWidth(Romanizer.romanize(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.

It would be better if we cover many patterns (ヘボン式、訓令式 etc.), but we may postpone that for future work. Please create an issue about this if so.

@katsuta-a-wap
katsuta-a-wap force-pushed the feature/completion-filter-develop branch from 98ff305 to dca4243 Compare August 7, 2026 09:40
@katsuta-a-wap

Copy link
Copy Markdown
Author

@hayashi-mas-wap
Address review comments on SudachiCompletionFilter

  • Take surface from CharTermAttribute so upstream filters such as normalized_form are respected.
  • Handle missing MorphemeAttribute by skipping tokens without a morpheme.
  • Simplify query-mode concatenation logic: merge kana+alphabet with the general non-alphabet+alphabet immediate-output case. And, added comments describing the expected specifications for each when-case pattern.
  • Set MorphemeAttribute on emitted tokens; clear it for romanized or concatenated tokens where no single morpheme applies.
  • Add README documentation for the sudachi_completion token filter.

@hayashi-mas-wap
hayashi-mas-wap self-requested a review August 24, 2026 03:16
val m: Morpheme? = morphemeAtt.getMorpheme()
// If the upstream filter did not set a morpheme, there is nothing we can add to the
// completion generator, so skip this input token.
if (m == null) {

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.

Skipping null-morpheme token seems dangerous. It may unexpectedly drop intermediate token (although our other non-graph token filters won't produce such case).

We should keep processing with reading = null or "" this case.


if (token.morpheme != null) {
morphemeAtt.setMorpheme(token.morpheme)
morphemeAtt.setOffsets(token.offsetMap)

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.

morphemeAtt.offset is NOT equals to offsetAtt (see e.g.

morphemeAtt.setOffsets(offsetMap.subList(su.begin, su.end + 1));
offsetAtt.setOffset(correctOffset(su.begin), correctOffset(su.end));
).
we need to keep it in tokenGenerator with morpheme (maybe we can have data class for them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants