-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Refactors PrefixQuery #12032
Refactors PrefixQuery #12032
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/* | ||
* Licensed to Elasticsearch under one or more contributor | ||
* license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright | ||
* ownership. Elasticsearch licenses this file to you under | ||
* the Apache License, Version 2.0 (the "License"); you may | ||
* not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.elasticsearch.index.query; | ||
|
||
import org.apache.lucene.index.Term; | ||
import org.apache.lucene.search.MultiTermQuery; | ||
import org.apache.lucene.search.PrefixQuery; | ||
import org.apache.lucene.search.Query; | ||
import org.elasticsearch.common.ParseFieldMatcher; | ||
import org.elasticsearch.common.lucene.BytesRefs; | ||
import org.elasticsearch.index.mapper.MappedFieldType; | ||
import org.elasticsearch.index.query.support.QueryParsers; | ||
import org.junit.Test; | ||
|
||
import java.io.IOException; | ||
|
||
import static org.hamcrest.Matchers.is; | ||
|
||
public class PrefixQueryBuilderTest extends BaseQueryTestCase<PrefixQueryBuilder> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if it's possible to also extend the generic BaseTermQueryTestCase here, since a lot of the setup looks similar. That one is also testing Boolean as a value type while this test adds Date there. Maybe the creation of the expected query gets a bit too complicated, but the two existing Test classes (SpanTermQueryBuilderTest, TermQueryBuilderTest) are quiet small that way. Might be worth looking into merging, unless you already tried and found it to hard to read afterwards. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patially revoking my last comment, I would only try this and add the inheritance level in the test if in the end you decide to have PrefixQueryBuilder extend BaseTermQueryBuilder. Thinking about it a bit more, I'm not sure if this is maybe growing to complex at this point and a bit of code duplication should be traded for ease of understanding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for using BaseTermQueryTestCase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if PrefixQueryBuilder won't extend BaseTermQueryBuilder anymore I think we can't extend BaseTermQueryTestCase either here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be possible, but I would try to avoid it in this case. I would go for either using both BaseTerm classes or none. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK then we go for none? I am ok either way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave it as-is, it needs to extend BaseQueryTestCase |
||
|
||
@Override | ||
protected PrefixQueryBuilder doCreateTestQueryBuilder() { | ||
String fieldName = randomBoolean() ? STRING_FIELD_NAME : randomAsciiOfLengthBetween(1, 10); | ||
String value = randomAsciiOfLengthBetween(1, 10); | ||
PrefixQueryBuilder query = new PrefixQueryBuilder(fieldName, value); | ||
|
||
if (randomBoolean()) { | ||
query.rewrite(getRandomRewriteMethod()); | ||
} | ||
return query; | ||
} | ||
|
||
@Override | ||
protected Query doCreateExpectedQuery(PrefixQueryBuilder queryBuilder, QueryParseContext context) throws IOException { | ||
//norelease fix to be removed to avoid NPE on unmapped fields (Dtests.seed=BF5D7566DECBC5B1) | ||
context.parseFieldMatcher(randomBoolean() ? ParseFieldMatcher.EMPTY : ParseFieldMatcher.STRICT); | ||
|
||
MultiTermQuery.RewriteMethod method = QueryParsers.parseRewriteMethod(context.parseFieldMatcher(), queryBuilder.rewrite(), null); | ||
|
||
Query query = null; | ||
MappedFieldType fieldType = context.fieldMapper(queryBuilder.fieldName()); | ||
if (fieldType != null) { | ||
query = fieldType.prefixQuery(queryBuilder.value(), method, context); | ||
} | ||
if (query == null) { | ||
PrefixQuery prefixQuery = new PrefixQuery(new Term(queryBuilder.fieldName(), BytesRefs.toBytesRef(queryBuilder.value()))); | ||
if (method != null) { | ||
prefixQuery.setRewriteMethod(method); | ||
} | ||
query = prefixQuery; | ||
} | ||
|
||
return query; | ||
} | ||
|
||
@Test | ||
public void testValidate() { | ||
PrefixQueryBuilder prefixQueryBuilder = new PrefixQueryBuilder("", "prefix"); | ||
assertThat(prefixQueryBuilder.validate().validationErrors().size(), is(1)); | ||
|
||
prefixQueryBuilder = new PrefixQueryBuilder("field", null); | ||
assertThat(prefixQueryBuilder.validate().validationErrors().size(), is(1)); | ||
|
||
prefixQueryBuilder = new PrefixQueryBuilder("field", "prefix"); | ||
assertNull(prefixQueryBuilder.validate()); | ||
|
||
prefixQueryBuilder = new PrefixQueryBuilder(null, null); | ||
assertThat(prefixQueryBuilder.validate().validationErrors().size(), is(2)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also reuse the BaseTermQueryBuilder#validate here? I realize we agreed on always overwriting this for the AbstractQueryBuilder, but since this just repeats the super types implementation, maybe we should call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely! +1