Refactoring of FuzzyQuery by alexksikes · Pull Request #11865 · elastic/elasticsearch

alexksikes · 2015-06-24T22:01:12Z

Relates to #10217

This PR is against the query-refactoring branch.

MaineC · 2015-06-25T07:31:34Z

core/src/main/java/org/elasticsearch/index/query/FuzzyQueryParser.java

Can you please add some text or remove the empty comment?

cbuescher · 2015-06-25T14:00:17Z

Did a round of review, agree with most of the changes. I left some comments, mostly concerning questions of where to set default Rewrite method when in filter context and some thoughts concerning leaving internal query value representation as Object. We might have to check if it is possible to restrict the Java-API to only accept Numeric, String and Date values (at least that's what seems allowed reading the docs). Then we might be able to just have one internal String representation.

alexksikes · 2015-06-25T20:32:56Z

Thanks for the review. I went for storing value as a String internally. I'm not entirely sure this is the right decision though. We should probably only accept Numeric, String and Date values.

cbuescher · 2015-06-26T13:50:40Z

@alexksikes I went through your last commit, left a few comments. Sorry I didn't realize they show up in such a strange way if I do the comments on the commit directly here in Github, will try to avoid this in the future.

cbuescher · 2015-06-29T09:35:43Z

core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java

Since value internally is a String now, we can change read/write here as well.

cbuescher · 2015-06-29T10:21:24Z

Went through the changes another time, would like for @javanna to have another look and for an opinion about how much we should restrict the current constructor that allows (but won't work) for any Object argument

alexksikes · 2015-06-29T17:25:18Z

Thanks for the review. I restricted the set of accepted values for now.

javanna · 2015-07-02T08:27:06Z

core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java

maybe add a new constructor Fuzziness(Object) instead of these ifs?

@alexksikes did you see this comment?

Not sure why it is rendered like this in github, but I am using a factory Fuzziness.newFuziness.

I see but I wonder if we should remove this method and simply accept Object and add a Fuzziness constructor that accepts Object instead

javanna · 2015-07-02T08:45:19Z

I had a quick look at this and left a few comments. There is an inconsistency between parser and builder here in the existing code. The builder supports an object for the value mamber but the parser only parses a string. I am not 100% sure which of the two is wrong. Feels like fuzzy query should allow for an object similar to what the term query does, but then our MappedFieldType#fuzzyQuery method takes a string as argument, so object wouldn't really work out, that would lead us to support strings only then? Thoughts @jpountz ?

javanna · 2015-07-02T08:46:42Z

Also @alexksikes would be nice if you can rebase, it is hard to review at this point after #11974 got in. Thanks!

jpountz · 2015-07-02T09:47:30Z

@javanna I looked at how classes that extend MappedFieldType implement fuzzyQuery, and they seem to all need to perform some parsing of the string value. So maybe we could make it an object and update all fuzzyQuery implementations to parse the object to the appropriate type (Date, Long, ...) using exactly the same logic as the termQuery method?

javanna · 2015-07-02T09:48:36Z

Agreed @jpountz thanks for looking. We will do that upstream rather than in the query-refactoring branch then. Makes sense @alexksikes ?

jpountz · 2015-07-02T09:48:56Z

+1 to doing it in master

alexksikes · 2015-07-03T12:08:28Z

Yes agreed. Thanks @jpountz.

javanna · 2015-08-04T07:46:18Z

core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java

can you explain the reason behind this change?

just the ordering changed

javanna · 2015-08-04T07:52:29Z

left come comments

alexksikes · 2015-08-10T10:00:37Z

@javanna I addressed the comments and rebased. Thanks for the review.

javanna · 2015-08-10T11:23:38Z

core/src/test/java/org/elasticsearch/test/ESTestCase.java

didn't we say that we would change this?

I thought you meant between 0 - 3?

no I meant that I don't understand why we randomize between 0 and 3. Can you elaborate?

yes I guess that is more of a fantasy related to the name of the method, although 0s or 0d could be corner cases.

Lemme clarify I mean that if we add this method to ESTestCase it should be generic enough. I guess there are much more valid values possible for a time value?

OK then the value will be 0 - 1000

javanna · 2015-08-10T11:26:41Z

I did another round and replied to your comments. @cbuescher wanna have a quick look too?

cbuescher · 2015-08-10T13:24:58Z

core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java

I think those two fields are now non-optional, so could use readInt()?

since these are more likely to be small, better use the V methods. There are always written and yes there are none optional.

Oh, I'm sorry I mixed VInt with optional, my fault. Must be the heat. Please ignore.

No worries, the heat does affect me too :-)

cbuescher · 2015-08-10T13:36:13Z

@alexksikes did a very quick round and left only a few comments, mostly small stuff and questions for clarfication

alexksikes · 2015-08-10T14:42:49Z

@javanna rebased and comments addressed. Thanks for the review as always.

javanna · 2015-08-10T14:46:12Z

core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java

this could be replaced with Objects.requireNotNull

i see it throws an NPE exception, not as neat as the error we return to the user, unless you want to wrap the call in a try catch and still return the nicer error to the user.

javanna · 2015-08-10T14:51:47Z

left a small comment

javanna · 2015-08-10T15:04:47Z

core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java

I meant to leverage our own equals method rather than a string comparison. AUTO.equals(this) ?

javanna · 2015-08-10T15:19:43Z

LGTM

Relates to #10217 This PR is against the query-refactoring branch. Closes #11865

alexksikes added :Query Refactoring labels Jun 24, 2015

MaineC reviewed Jun 25, 2015
View reviewed changes

clintongormley mentioned this pull request Jun 26, 2015

Refactor parsing of queries/filters, aggs, suggester APIs #10217

Closed

cbuescher reviewed Jun 29, 2015
View reviewed changes

javanna reviewed Jul 2, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-fuzzy branch 2 times, most recently from 5ac4b5f to 5f655f5 Compare July 3, 2015 12:19

javanna reviewed Aug 4, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-fuzzy branch from f196754 to 45e6afd Compare August 10, 2015 10:00

javanna reviewed Aug 10, 2015
View reviewed changes

cbuescher reviewed Aug 10, 2015
View reviewed changes

alexksikes force-pushed the feature/query-refactoring-fuzzy branch from 3ebce4b to fae6cb7 Compare August 10, 2015 14:41

javanna reviewed Aug 10, 2015
View reviewed changes

alexksikes added a commit that referenced this pull request Aug 10, 2015

Refactoring of FuzzyQuery

626b1ab

Relates to #10217 This PR is against the query-refactoring branch. Closes #11865

alexksikes closed this Aug 10, 2015

alexksikes force-pushed the feature/query-refactoring-fuzzy branch from f96badb to 626b1ab Compare August 10, 2015 15:27

alexksikes deleted the feature/query-refactoring-fuzzy branch August 10, 2015 15:27

alexksikes removed the review label Sep 14, 2015

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018

Conversation

alexksikes commented Jun 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jun 25, 2015

Uh oh!

alexksikes commented Jun 25, 2015

Uh oh!

cbuescher commented Jun 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Jun 29, 2015

Uh oh!

alexksikes commented Jun 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jul 2, 2015

Uh oh!

javanna commented Jul 2, 2015

Uh oh!

jpountz commented Jul 2, 2015

Uh oh!

javanna commented Jul 2, 2015

Uh oh!

jpountz commented Jul 2, 2015

Uh oh!

alexksikes commented Jul 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 4, 2015

Uh oh!

alexksikes commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbuescher commented Aug 10, 2015

Uh oh!

alexksikes commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Aug 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment