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

Refactoring of FuzzyQuery #11865

Conversation

@alexksikes
Copy link
Contributor

commented Jun 24, 2015

Relates to #10217

This PR is against the query-refactoring branch.

@MaineC
MaineC reviewed Jun 25, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryParser.java Outdated
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.unit.Fuzziness;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.query.support.QueryParsers;

import java.io.IOException;

/**
*
*/

This comment has been minimized.

Copy link
@MaineC

MaineC Jun 25, 2015

Contributor

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

@MaineC
MaineC reviewed Jun 25, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTest.java Outdated
}

@Override
protected Query createExpectedQuery(FuzzyQueryBuilder queryBuilder, QueryParseContext context) throws IOException {

This comment has been minimized.

Copy link
@MaineC

MaineC Jun 25, 2015

Contributor

Much like with the tests for SimpleQueryStringQuery this method is almost a copy of the original toQuery code in the QueryBuilder. Can we track the reasoning behind this type of testing here: https://github.com/elastic/dev/blob/master/design/queries/general-guidelines.md (I believe we've discussed it in multiple PR threads and face2face but have lost of all the specific arguments).

This comment has been minimized.

Copy link
@javanna

javanna Jun 25, 2015

Member

I will take care of this.

@MaineC
MaineC reviewed Jun 25, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated
* Sets the boost for this query. Documents matching this query will (in addition to the normal
* weightings) have their score multiplied by the boost provided.
*/
@Override

This comment has been minimized.

Copy link
@MaineC

MaineC Jun 25, 2015

Contributor

I think we can simply re-use the comment of the super method by leaving this one undocumented?

@MaineC
MaineC reviewed Jun 25, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated

public static final String NAME = "fuzzy";

private final String name;
public static final Fuzziness DEFAULT_FUZZINESS = Fuzziness.AUTO;

This comment has been minimized.

Copy link
@MaineC

MaineC Jun 25, 2015

Contributor

Personally I like having at least a brief line of JavaDoc on anything that is public that explains it's purpose...

@cbuescher
cbuescher reviewed Jun 25, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated

static MultiTermQuery.RewriteMethod parseRewriteString(QueryParseContext parseContext, @Nullable String rewrite) {
MultiTermQuery.RewriteMethod rewriteMethod = null;
if (parseContext.isFilter()) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 25, 2015

Member

I think the filter flag needs to be checked earlier, already in the parser. isFilter is a statefull flag in the QueryParseContext that gets set/unset when parsing one of the (former) filters. ParseContext.isFilter at this point doesn't reflect the state it was in during traversing the query tree. Check QueryParseContext.parseInnerFilterToQueryBuilder() to see where state changes can occur.

@cbuescher
cbuescher reviewed Jun 25, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTest.java Outdated
}
expectedQuery.setBoost(queryBuilder.boost());

if (queryBuilder.queryName() != null) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 25, 2015

Member

I think we shouldn't add the expectedQuery to the namedQuery in the context here, that should be done in toQuery() and it's part of what's beeing tested later.

@cbuescher
cbuescher reviewed Jun 25, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated

private Integer maxExpansions;
private final Object text;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 25, 2015

Member

If this stays an Object we have a problem with builder equality when we do the roundtrip doXContent->fromXContent that we currently do in the tests. I had similar problems in TermQueryBuilder already. Imagine an Integer (126) that is rendered to JSON (or other XContent) via writeValue(Object value) which inspects the class of the Object and then chooses a type-specific method to write it. On the parser side, this is always read back as a String ("126") with parser.text(). So if we check those builders for equality we will fail with 126 != "126". For the lucene query produced in the end this difference won't matter (we convert all values to String there), but I think it would be great to keep that property that when we have builder A, generate XContent for that and parse it to Builder B, they should be the same.
One option would be to push up the String-conversion we later do in toQuery() anyway to the constructor. I'm not sure if the java API currently is to lenient here by allowing any Object as argument, since parsing only works for object types that correctly parse back as String.
Another option might be to allow parsing to numeric values using objectText() like in CommonTermsQueryParser. Not sure about which way is better to be honest.

@cbuescher

This comment has been minimized.

Copy link
Member

commented Jun 25, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2015

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

This comment has been minimized.

Copy link
Member

commented Jun 26, 2015

@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
cbuescher reviewed Jun 29, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated
@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(this.fieldName);
out.writeGenericValue(this.value);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 29, 2015

Member

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

@cbuescher
cbuescher reviewed Jun 29, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated
public FuzzyQueryBuilder(String name, Object value) {
this.name = name;
this.value = value;
public FuzzyQueryBuilder(String fieldName, Object value) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 29, 2015

Member

If we already break the builder API by changing the value-getter, it would be great to somehow also restrict the constructor to not accept every value type anymore. Maybe deprecate this constructor and instroduce new ones for String and Number? Not sure if also Date type is handled correctly when parsed to String here.
In general I'm unsure here to how far this PR should change the status quo. I think only selected Object types worked here in the past, it should be stated somehere (docs?) what actually works. In our own code we really only use the constructor with String argument, so at least internally we could easily change that, but not sure how many client code this would break.

This comment has been minimized.

Copy link
@javanna

javanna Jul 10, 2015

Member

don't we need constructors for other supported type slike we do in term query?

This comment has been minimized.

Copy link
@javanna

javanna Jul 10, 2015

Member

sorry didn't see them, can you move them up please?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Jul 10, 2015

Author Contributor

sure

@cbuescher
cbuescher reviewed Jun 29, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryParser.java Outdated
MultiTermQuery.RewriteMethod rewriteMethod = null;
if (parseContext.isFilter()) {
rewriteMethod = MultiTermQuery.CONSTANT_SCORE_REWRITE;
}

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 29, 2015

Member

From the discussion before I think that removing the special case for FuzzyQuery inside filter context is okay here since CONSTANT_SCORE_REWRITE ist the default MultiTermQuery rewrite method anyway.
@jpountz could you have a look at this change since I think it's related to the filter/query merging.

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 29, 2015

Contributor

CONSTANT_SCORE_REWRITE is the rewrite for all multi-term queries but FuzzyQuery: see the FuzzyQuery constructor which calls setRewriteMethod(new MultiTermQuery.TopTermsBlendedFreqScoringRewrite(maxExpansions));

This comment has been minimized.

Copy link
@cbuescher

cbuescher Jun 29, 2015

Member

Thanks, seems that unfortunately we need to re-set it then when we're in filter context. @alexksikes I guess that means we have to go back to using the String constants from QueryParser#parseRewriteMethod. I was wondering if it makes sense to factor out the constants used there into a separate enum or a fixed list of constants to make the dependency of the Parser to those constants more explicit. I see that the second part of QueryParser#parseRewriteMethod also does some suffix-magix with the values passed in, but at least for the constant we use in FuzzyQueryParser it would be good to link it somehow to the code there. Or whats your take on this?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Jun 29, 2015

Author Contributor

OK we should only rewrite if we have a filter or if rewrite is explicitly set, otherwise the default rewrite on FuzzyQuery is untouched. If we want to make the rewrite options as constants, we would have to do so for every query that supports rewrite. Not sure this is really necessary.

@cbuescher

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2015

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

@javanna
javanna reviewed Jul 2, 2015
View changes
core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java Outdated
} else if (value instanceof Float) {
return new Fuzziness((Float) value);
} else if (value instanceof String) {
return new Fuzziness((String) value);

This comment has been minimized.

Copy link
@javanna

javanna Jul 2, 2015

Member

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

This comment has been minimized.

Copy link
@javanna

javanna Jul 10, 2015

Member

@alexksikes did you see this comment?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Jul 10, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@javanna

javanna Jul 10, 2015

Member

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
javanna reviewed Jul 2, 2015
View changes
core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java Outdated
@@ -106,9 +115,12 @@ public static Fuzziness parse(XContentParser parser) throws IOException {
return build(fuzziness);
}
} catch (NumberFormatException ex) {
return build(fuzziness);
try {

This comment has been minimized.

Copy link
@javanna

javanna Jul 2, 2015

Member

can you explain the reason behind this change?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Jul 3, 2015

Author Contributor

This is to properly parse floats into floats and not into strings, so that the query from the parser is the same as from the builder. I would have errors such as "0.6" != 0.6.

@javanna
javanna reviewed Jul 2, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated

/**
* A Query that does fuzzy matching for a specific value.
*/
public class FuzzyQueryBuilder extends MultiTermQueryBuilder implements BoostableQueryBuilder<FuzzyQueryBuilder> {
public class FuzzyQueryBuilder extends MultiTermQueryBuilder<FuzzyQueryBuilder> implements BoostableQueryBuilder<FuzzyQueryBuilder> {

This comment has been minimized.

Copy link
@javanna

javanna Jul 2, 2015

Member

implements BoostableQueryBuilder should be gone once you rebase

@javanna
javanna reviewed Jul 2, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated

private String rewrite;

private float boost = 1.0f;

This comment has been minimized.

Copy link
@javanna

javanna Jul 2, 2015

Member

boost will be gone once you rebase, same for queryName

@javanna

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

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

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

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

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

@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

This comment has been minimized.

Copy link
Member

commented Jul 2, 2015

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

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2015

+1 to doing it in master

@alexksikes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2015

Yes agreed. Thanks @jpountz.

@alexksikes alexksikes force-pushed the alexksikes:feature/query-refactoring-fuzzy branch 2 times, most recently Jul 3, 2015
@javanna
javanna reviewed Aug 4, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java Outdated
INT_FIELD_NAME, "type=integer",
DOUBLE_FIELD_NAME, "type=double",
BOOLEAN_FIELD_NAME, "type=boolean",
STRING_FIELD_NAME, "type=string",
DATE_FIELD_NAME, "type=date",

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2015

Member

can you explain the reason behind this change?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

just the ordering changed

@javanna
javanna reviewed Aug 4, 2015
View changes
core/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java Outdated

public static String randomTimeValue() {
final String[] values = new String[]{"d", "H", "ms", "s", "S", "w"};
return randomIntBetween(1, 3) + randomFrom(values);

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2015

Member

why between 1 and 3 ?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

sure this could be 0d, 0H etc .. will change

@javanna
javanna reviewed Aug 4, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java Outdated
/**
* Helper method to return a random field (mapped or unmapped) and a value
*/
protected static Tuple<String, Object> getRandomFieldNameAndValue() {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2015

Member

I think that this method duplicates logic that's already existing in other tests. If so can we use it in other tests too?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

yes but it would be better to make this change in another PR?

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

potentially but I don't see why we add a method in the base class if we don't use it everywhere. then add it to the class that needs it only?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

I can move it back to FuzzyQueryBuilderTest. But note that many queries could make use of this starting with Term Query.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

then let's use it now otherwise this change is half baked

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

wouldn't it be better to make the change only local to FuzzyQuery, and then have another PR in which we make use of this method for all the other queries? In which case I'm happy to move it back to FuzzyQueryBuilderTest.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

I think it's a simple enough change to make as part of this PR.

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

ok we settled for doing it in another PR, thanks @javanna

@javanna
javanna reviewed Aug 4, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java Outdated
return fuzziness;
}

protected static boolean isNumericFieldName(String fieldName) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2015

Member

to be honest I think we have enough static helper methods in the test base class, can we simply check the name of the field in the fuzzy test that needs this?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

this is more for readability, also there are other types which are numerical which in the future we might want to check against. I can still make that change if you'd like though.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

ok lets keep it but please let's not create a list for this, we can just do if INT.equals(field) || DOUBLE.equals(field)

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

sure thanks, that seems better.

@javanna
javanna reviewed Aug 4, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/BaseQueryTestCase.java Outdated
return new Tuple(fieldName, value);
}

protected static Fuzziness randomFuzziness(String fieldName) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 4, 2015

Member

what's the reason for this to be in the test base class? is it going to be used in other tests?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

A couple of other queries have a fuzziness option, so it will be handy to have them share this part of the code.

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

left come comments

@alexksikes alexksikes force-pushed the alexksikes:feature/query-refactoring-fuzzy branch Aug 10, 2015
@alexksikes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

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

@javanna
javanna reviewed Aug 10, 2015
View changes
core/src/test/java/org/elasticsearch/test/ESTestCase.java Outdated

public static String randomTimeValue() {
final String[] values = new String[]{"d", "H", "ms", "s", "S", "w"};
return randomIntBetween(0, 3) + randomFrom(values);

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

didn't we say that we would change this?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

I thought you meant between 0 - 3?

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

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

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

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?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

OK then the value will be 0 - 1000

@javanna
javanna reviewed Aug 10, 2015
View changes
core/src/test/java/org/elasticsearch/index/query/FuzzyQueryBuilderTest.java Outdated
context.setAllowUnmappedFields(true);

FuzzyQueryBuilder fuzzyQueryBuilder = new FuzzyQueryBuilder(STRING_FIELD_NAME, "text");
fuzzyQueryBuilder.fuzziness(Fuzziness.build(randomFrom("a string which is not auto", "3h", "200s")));

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

can we not validate this situation before toQuery? where does the exception get thrown at the moment exactly?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

Those are field type dependant. For example, a string field can't use fuzziness with a time distance. I could check for a fuzziness string which is not "AUTO" though.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

where is the NumberFormatException thrown?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

this happens because the fieldType is not of the correct type when performing this fuzzy query. For string fields, we can only have fuzziness be an edit distance, not a time distance nor a String other than AUTO. So the asInt() method of Fuzziness is called and calls as a last resort Float.parseFloat (meaning maybe the user tried to type 1.0) which then returns a NumberFormatException.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

ok thanks for the explanation. I guess if we can detect this earlier and add some intelligence to the validate method?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

i'd think we would need the fieldType in validate for that

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

right, maybe then add some clearer validation in toQuery given the filedType ? I don't love this NumberFormatException, would love to have clearer error messages.

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

we agreed that this is not that easy to do as it depends on mappings, we will leave as-is for now

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

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

@cbuescher
cbuescher reviewed Aug 10, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated
FuzzyQueryBuilder fuzzyQueryBuilder = new FuzzyQueryBuilder(in.readString(), in.readGenericValue());
fuzzyQueryBuilder.fuzziness = Fuzziness.readFuzzinessFrom(in);
fuzzyQueryBuilder.prefixLength = in.readVInt();
fuzzyQueryBuilder.maxExpansions = in.readVInt();

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 10, 2015

Member

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

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 10, 2015

Member

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

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

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

@cbuescher
cbuescher reviewed Aug 10, 2015
View changes
core/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java Outdated
out.writeGenericValue(this.value);
this.fuzziness.writeTo(out);
out.writeVInt(this.prefixLength);
out.writeVInt(this.maxExpansions);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 10, 2015

Member

same as above

This comment has been minimized.

Copy link
@cbuescher

cbuescher Aug 10, 2015

Member

sorry, my bad.

@cbuescher

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

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

@alexksikes alexksikes force-pushed the alexksikes:feature/query-refactoring-fuzzy branch Aug 10, 2015
@alexksikes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

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

@javanna
javanna reviewed Aug 10, 2015
View changes
core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java Outdated
private Fuzziness(int fuzziness) {
Preconditions.checkArgument(fuzziness >= 0 && fuzziness <= 2, "Valid edit distances are [0, 1, 2] but was [" + fuzziness + "]");
this.fuzziness = Integer.toString(fuzziness);
}

private Fuzziness(String fuzziness) {
this.fuzziness = fuzziness;
if (fuzziness == null) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

this could be replaced with Objects.requireNotNull

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

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
javanna reviewed Aug 10, 2015
View changes
core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java Outdated
return 1f;
}
return Float.parseFloat(fuzziness.toString());
}

// A deserialized fuzziness with AUTO is not the static Fuzziness.AUTO object
private boolean isAuto() {

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

can't this method go away now?can't we just do AUTO.equals(this) ?

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

sure but isAuto() reads better no?

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

left a small comment

@javanna
javanna reviewed Aug 10, 2015
View changes
core/src/main/java/org/elasticsearch/common/unit/Fuzziness.java Outdated
@@ -121,7 +130,7 @@ public int asDistance() {
}

public int asDistance(String text) {
if (this == AUTO) { //AUTO
if (fuzziness.equals("AUTO")) { //AUTO

This comment has been minimized.

Copy link
@javanna

javanna Aug 10, 2015

Member

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

This comment has been minimized.

Copy link
@alexksikes

alexksikes Aug 10, 2015

Author Contributor

ha yes!

@javanna

This comment has been minimized.

Copy link
Member

commented Aug 10, 2015

LGTM

alexksikes added a commit that referenced this pull request Aug 10, 2015
Relates to #10217

This PR is against the query-refactoring branch.

Closes #11865
@alexksikes alexksikes closed this Aug 10, 2015
@alexksikes alexksikes force-pushed the alexksikes:feature/query-refactoring-fuzzy branch to 626b1ab Aug 10, 2015
@alexksikes alexksikes deleted the alexksikes:feature/query-refactoring-fuzzy branch Aug 10, 2015
@alexksikes alexksikes removed the review label Sep 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.