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

Deserialization issue with value size #251

Closed
georgemcz opened this issue Oct 1, 2024 · 8 comments
Closed

Deserialization issue with value size #251

georgemcz opened this issue Oct 1, 2024 · 8 comments

Comments

@georgemcz
Copy link
Contributor

Under some circumstances, we receive an exception during multi-search. I found that it is an issue with the extreme number of dropped tokens during the search - it fails when Hit.TextMatchInfo.NumTokensDropped shall contain deserialized json value:

"num_tokens_dropped":18446744073709551615 from text_match_info

which clearly does not fit into the int type. It seems that the typesense itself allows there some larger range.

Mitigation - bump the type to long, optionally also for other num_ types, this fix seems to work.

public record TextMatchInfo  
{ 
    //...

    [JsonPropertyName("num_tokens_dropped")]  
    public long NumTokensDropped { get; set; }     // changed from int to long  
    
    //...
}
@runeanielsen
Copy link
Member

I think this is fine, you're welcome to make a PR with the change.

@runeanielsen
Copy link
Member

@georgemcz this has now been released in version 7.20.0.

@adonistseriotis
Copy link
Contributor

@runeanielsen Hey, this error has been occurring to me as well. I figured that long is not enough. It needs ulong.

@georgemcz
Copy link
Contributor Author

Could be that you are dropping more tokens 😀
Shall I bump it, or you will?

@adonistseriotis
Copy link
Contributor

adonistseriotis commented Oct 3, 2024

Could be that you are dropping more tokens 😀
Shall I bump it, or you will?

Can I configure it somehow from the cloud dashboard?
I have applied the change locally for my use case to work. I can also pull request the change as well.
Nevertheless 'ulong' should capture all use cases.

@runeanielsen
Copy link
Member

I am fine with switching it to ulong, feel free to make a PR.

@runeanielsen runeanielsen reopened this Oct 4, 2024
@georgemcz
Copy link
Contributor Author

Done. Just forced to do it over web gui, hopefully all is ok

@runeanielsen
Copy link
Member

This has now been released in version 7.21.0.

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

No branches or pull requests

3 participants