-
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
Conversation
|
||
@Override | ||
protected PrefixQueryBuilder doCreateTestQueryBuilder() { | ||
PrefixQueryBuilder query = newRandomQueryBuilder(); |
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.
out of curiosity, why do we have a newRandomQueryBuilder
method here?
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.
I was actually thinking that this could later be moved as a helper method in the BaseQueryTestCase class, something that returns a random field name together with its value.
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.
ah ok I see
left a couple of small comments |
protected void doWriteTo(StreamOutput out) throws IOException { | ||
out.writeString(fieldName); | ||
out.writeGenericValue(value); | ||
out.writeOptionalString(fieldName); |
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.
Should this be rewrite
field instead of fieldName
(mentioned twice)
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.
Thanks good catch!
@javanna @cbuescher Thanks for the review. I updated the PR accordingly. |
looks good to me, @cbuescher can you have a final look plese? |
} | ||
value = randomDate(); | ||
break; | ||
default: |
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.
This can't happen, and if for some reason after the switch fielname/value are unassigned the returned query will throw exceptions at some point (validation, doXContent, toQuery), so given this is a test method I'd remove it.
Did another round of reviews, left just two question/comments regarding setup of test query, otherwise looks good. |
The parser takes an Object value, so should the builder. Relates to elastic#12032
b882a2d
to
0aaac85
Compare
@javanna @cbuescher Thanks for the review. I rebased the PR and updated it. |
} | ||
|
||
@Override | ||
public QueryValidationException validate() { |
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
left a couple of minor comments, I would wait for #12124 (or corresponding adapted PR) before getting this in though. |
|
||
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave it as-is, it needs to extend BaseQueryTestCase
Also did another round and left a couple of comments. |
0aaac85
to
4997b9e
Compare
@javanna Thanks for the review. I rebased and updated the PR accordingly. |
return Objects.equals(fieldName, other.fieldName) && | ||
Objects.equals(value, other.value); | ||
} | ||
|
||
@Override | ||
protected final QB doReadFrom(StreamInput in) throws IOException { |
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.
we can revert these two changes no given that we dont' extend from this class anymore?
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.
I think we should allow these to be overridden, but I'm happy to make the change.
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.
well we don't need to extend this anywhere, plus these changes don't have anything to do with this PR at this point, would prefer to leave them out.
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.
sure I'll revert this change.
@javanna Thanks for the review. We need to address #11865 (comment) before it can be pushed though, and waiting on #12204 as well. |
Relates to elastic#10217 Closes elastic#12032 This PR is against the query-refactoring branch.
8a75083
to
37c6347
Compare
@javanna it's rebased, you can take a look. Thank you. |
LGTM |
Relates to elastic#10217 Closes elastic#12032 This PR is against the query-refactoring branch.
Relates to #10217
This PR is against the query-refactoring branch.