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

Fuzziness alignment for MatchQueryDescriptor #941

Closed
wants to merge 6 commits into from
Closed

Fuzziness alignment for MatchQueryDescriptor #941

wants to merge 6 commits into from

Conversation

andersosthus
Copy link
Contributor

Allows a Match query to take one of the following for Fuzziness:

  • .Fuzziness() (for Auto)
  • .Fuzziness(double)
  • .Fuzziness(int)

This matches the docs at http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/query-dsl-match-query.html#_boolean

I've seen that there are two ways this kind of fuzziness is implemented in Nest.
There is this style that's used in some of the Suggestors, like the FuzzySuggestDescriptor: https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Nest/DSL/Suggest/FuzzySuggestDescriptor.cs

And then there is another style, basically taking all fuzziness values and calling .ToString() on it so they can all fit in a String field. This is used in the FuzzyQueryDescriptor: https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Nest/DSL/Query/FuzzyQueryDescriptor.cs

I feel the IFuzziness way is cleaner :)

This PR does not break anything, since the signature still matches the old signature. The main thing I don't like is the changes I had to do to MatchQueryJsonConverter.cs, to have the Fuzziness deserialize properly. I see that there is a FuzzinessConverter already that does the same thing, but not sure how I could use that from the ReadJson in the MatchQueryJsonConverter.

@gmarz
Copy link
Contributor

gmarz commented Sep 11, 2014

Hey @andersosthus

I really like this and agree the IFuzziness approach is definitely cleaner. You're right that it doesn't break the fluent API, but it does unfortunately break the object initializer syntax since IMatchQuery.Fuzziness is changed from type double? to IFuzziness. Happy to merge this into the 2.0 branch though, so thank you!

In regards to the changes in MatchQueryJsonConverter, I think it would be best to centralize the logic that's already in FuzzinessConverter so that it's used in both converters. I'd have no problem with moving it into a public method inside FuzzinessConverter, newing it up in MatchQueryConverter and calling it there, or even making it static in FuzzinessConverter. Anything along those lines would be cool; as long as the same method is being used in both converters.

@andersosthus
Copy link
Contributor Author

Thanks @gmarz

I keep forgetting about the object initializers :\ So yeah, this will have to wait until 2.0 then.

I'll fix those things with the FuzzinessConverter and use that. Agreed that the logic should be shared, just wasn't sure if I could use some Json.net magic to just let it be fixed in some way :)

@gmarz
Copy link
Contributor

gmarz commented Sep 11, 2014

No worries at all. Having to always be mindful of bwc can be a real PITA, and it's really easy to forget sometimes ;).

That would be awesome, thanks again!

That I didn't need after all, but I left in :)
All logic in ```FuzzinessConverterHelper.ReadJson()```
Tests that exercise both the ```FuzzinessConverter``` and
```MatchQueryJsonConverter``` to test ```FuzzinessConverterHelper```
@andersosthus
Copy link
Contributor Author

@gmarz

I've updated the PR with some changes:

  • Added FuzzinessConverterHelper to hold the logic for IFuzziness deserialization.
  • FuzzinessConverter now calls FuzzinessConverterHelper.ReadJson() when deserializing.
  • MatchQueryJsonConverter now calls FuzzinessConverterHelper.ReadJson() when deserializing.
  • Added tests to make sure both paths work (mostly to keep me safe while refactoring :) )

The only thing I really don't like, is the object manipulation in FuzzinessConverter.ReadJson() to shape the data in the correct format for the FuzzinessConverterHelper.ReadJson()

Also not sure if the FuzzinessConverterHelper.ReadJson() should be safer, in case invalid data sneaks in, but I think the JTokenType checks should catch invalid datatypes.

Now the MatchQueryJsonConverter uses the FuzzinessConverter to
deserialize IFuzziness.
This removes the ugly object creation in FuzzinessConverter to get the
object into a unified format.
@andersosthus
Copy link
Contributor Author

Now I'm pretty happy with this :)

  • No more ugly JSON.Net object juggling in FuzzinessConverter.ReadJson()
  • FuzzinessConverterHelpergone
  • MatQueryJsonConverter now uses FuzzinessConverter to convert the IFuzziness

@gmarz
Copy link
Contributor

gmarz commented Sep 16, 2014

@andersosthus great work here- LGTM 👍

Thanks again for yet another awesome contribution!

@gmarz gmarz added the v2.0.0 label Sep 16, 2014
@gmarz gmarz closed this Sep 16, 2014
@Mpdreamz
Copy link
Member

FYI this was merged in our 2.0 branch:

7aaf289

@EmilAlipiev
Copy link

Is this fix not applied on 1.7? is it only for 2.0.x versions? I would like to know if there is any workaround on 1.7 version because 2.0 version has a lot of changes and it will take so much time to upgrade from 1.7 to 2.0 and I see that it is not very stable. Can you please advise? thanks

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.

None yet

4 participants