Refactoring of MissingQuery #12030
Refactoring of MissingQuery #12030
Conversation
| return INT_FIELD_NAME; | ||
| case 3: | ||
| return DOUBLE_FIELD_NAME; | ||
| default: |
There was a problem hiding this comment.
make this case 4 instead of default?
There was a problem hiding this comment.
the method needs to return though
There was a problem hiding this comment.
As this is only in the test I think we are save assuming that randomIntBetween can only return values between 0 and 3, so leaving the largest value as default should be ok. Current implementation is fine as well of course.
|
left two tiny comments, looks good though |
|
@javanna Thanks for the review. I updated the PR accordingly. |
| case 3: | ||
| return DOUBLE_FIELD_NAME; | ||
| } | ||
| return randomAsciiOfLengthBetween(1, 10); |
There was a problem hiding this comment.
before you would randomly return unmapped field names, now you never do. randomIntBetween(0, 4) was ok, case 4 was ok too, you can just throw UnsupportedOperationException from the default or something.
|
left a couple of minor comments, @cbuescher do you mind having a look too? |
| @@ -51,6 +69,10 @@ public MissingQueryBuilder nullValue(boolean nullValue) { | |||
| return this; | |||
| } | |||
|
|
|||
| public boolean nullValue() { | |||
There was a problem hiding this comment.
Reading just the method name, I was curious what it means in this context. Maybe short comment?
There was a problem hiding this comment.
good point, actually given that it's a public method some javadocs wouldn't hurt
There was a problem hiding this comment.
yes I'll add these, thanks for seeing this.
|
Also did a round of reviews, left some remarks. |
|
@javanna @cbuescher thanks for the review. I updated the PR accordingly. |
| QueryParseContext context = createContext(); | ||
| context.setAllowUnmappedFields(true); | ||
| try { | ||
| missingQueryBuilder.toQuery(context); |
There was a problem hiding this comment.
I would rather test this by calling the newFilter static method. In fact the check that we left there instead of relying on validate is there ony to protect other code paths from the outside (newFilter is public)
|
did another round, left a few small comments |
|
@javanna I updated the PR. Thanks. |
| this.name = name; | ||
| public MissingQueryBuilder(String fieldPattern) { | ||
| if (fieldPattern == null) { | ||
| throw new IllegalArgumentException("missing must be provided with a field"); |
There was a problem hiding this comment.
Does this work? Above PROTOTYPE is initialized with null which will result in an exception being thrown here.
|
Left a few comments. |
|
@MaineC Thanks for the review. I updated the PR. |
alexksikes
force-pushed
the
feature/query-refactoring-missing
branch
from
d9398ce to
64a50fd
Compare
|
@javanna It's rebased you can take a look. Thank you. |
|
|
||
| private Boolean nullValue = DEFAULT_NULL_VALUE; | ||
|
|
||
| private Boolean existence = DEFAULT_EXISTENCE_VALUE; |
There was a problem hiding this comment.
should we move to primitive booleans here?
There was a problem hiding this comment.
yes I think we can and should.
|
left one minor comment |
alexksikes
force-pushed
the
feature/query-refactoring-missing
branch
from
9f1e6e6 to
a4e9f64
Compare
Relates to elastic#10217 Closes elastic#12030 This PR is against the query-refactoring branch.
clintongormley
added
:Search/Search
Relates to #10217
This PR is against the query-refactoring branch.