Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to lang in ngrams_last_token_only_multi #1493

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

Joxit
Copy link
Member

@Joxit Joxit commented Oct 6, 2020

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀

@bboure found an issue in autocomplete multi-lang field, Edo Tokyo Museum is not returned see #1296 (comment)


Here's what actually got changed 👏

Since #1300 we are using two field for autocomplete search, name.default and name.{lang}. name.{lang} was missing in the ngrams_last_token_only_multi query


Here's how others can test the changes 👀

Edo Tokyo Museum should now work

@bboure
Copy link
Member

bboure commented Oct 6, 2020

Awesome! Thank you @Joxit

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Looks good to me.

It would be nice to have some tests for this in https://github.com/pelias/acceptance-tests to prevent regression.

@orangejulius
Copy link
Member

Thanks for fixing this @Joxit!

I was reading along with the comments yesterday and happened to write an acceptance test to examine this issue more closely, so I've opened pelias/acceptance-tests#535 to bring it in. Thanks @bboure for the report and test case as well!

@Joxit
Copy link
Member Author

Joxit commented Oct 7, 2020

Yeah, thx @orangejulius for the acceptance test 😄

@orangejulius orangejulius force-pushed the joxit/feat/multi-lang-ngram branch from 64b6210 to 9539912 Compare October 8, 2020 15:11
orangejulius added a commit to pelias/acceptance-tests that referenced this pull request Oct 8, 2020
@orangejulius orangejulius merged commit a8de5f0 into master Oct 8, 2020
@orangejulius orangejulius deleted the joxit/feat/multi-lang-ngram branch October 8, 2020 15:19
@missinglink
Copy link
Member

Heeeeey @Joxit

I was investigating a bug report today and I noticed something on our dev server which looks incorrect:
https://pelias.github.io/compare/#/v1/autocomplete?lang=de&text=Am+Gro%C3%9Fhausberg%2C+Furtwangen+im+Schwarzwald+deut&debug=1

Dev server is running a newer master branch than the production server.

Screenshot 2020-10-16 at 11 51 25

The screenshot shows it's using phrase.default but name.de, is that possibly due to this PR?
I would expect to see phrase.default and phrase.de.

@Joxit
Copy link
Member Author

Joxit commented Oct 16, 2020

Hi @missinglink Yes you're right, I think this is due to this PR 🤔

Maybe I should use admin:add_name_to_multimatch:field instead of admin:add_name_lang_to_multimatch:field...

@missinglink
Copy link
Member

It's not super urgent but we should fix it, could you please open an issue/pr so we don't forget?

@missinglink
Copy link
Member

Some of those query views are a hot mess, it's becoming increasingly difficult to figure out which does what, where.
I'm probably guilty of the worst of it, we need to clean all that up at some point 🤦

@Joxit
Copy link
Member Author

Joxit commented Oct 16, 2020

Ok, I will open an issue and work on it later (I tried something but it will be harder than I expected 😢)
Here is where we update the name.default field to phrase.default:

vsCopy.var('admin:add_name_to_multimatch:field', 'phrase.default');
adminFields.forEach(field => {
if( vsCopy.isset(`admin:${field}:field`) ){
vsCopy.var(`admin:${field}:field`, vsCopy.var(`admin:${field}:field`).get().replace('.ngram', ''));
}
});

😆 I agree 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants