Fix some type checks that were always false #27706
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
@rneatherway thanks for digging up more of these. I'll kick of a CI run first and will take a look in a bit. |
There was a problem hiding this comment.
@rneatherway I took a look, the first two changes look pretty straight forward to me but the last one I want to double check the impact of so I'm waiting for replies before pulling this in.
| @@ -75,9 +75,6 @@ void flatten(Query sourceQuery, IndexReader reader, Collection<Query> flatQuerie | |||
| } else if (sourceQuery instanceof BlendedTermQuery) { | |||
| final BlendedTermQuery blendedTermQuery = (BlendedTermQuery) sourceQuery; | |||
| flatten(blendedTermQuery.rewrite(reader), reader, flatQueries, boost); | |||
| } else if (sourceQuery instanceof ESToParentBlockJoinQuery) { | |||
| ESToParentBlockJoinQuery blockJoinQuery = (ESToParentBlockJoinQuery) sourceQuery; | |||
| flatten(blockJoinQuery.getChildQuery(), reader, flatQueries, boost); | |||
There was a problem hiding this comment.
+1,
This indeed looks similar to the later else-if branch which I would prefer for its null check. I only wonder if the order of clauses matters, but I checked BoostingQuery and SynonymQuery and they both don't seem to by supertypes of ESToParentBlockJoinQuery. Maybe @jimczi can check if I missed anything?
| return command; | ||
| } | ||
| Priority priority = ((PrioritizedRunnable) command).priority(); | ||
| return new TieBreakingPrioritizedRunnable(super.wrapRunnable(command), priority, insertionOrder.incrementAndGet()); | ||
| } else if (command instanceof PrioritizedFutureTask) { | ||
| return command; | ||
| } else { // it might be a callable wrapper... | ||
| if (command instanceof TieBreakingPrioritizedRunnable) { | ||
| return command; | ||
| } |
| @@ -618,16 +618,16 @@ private static void parseNullValue(ParseContext context, ObjectMapper parentMapp | |||
|
|
|||
| private static Mapper.Builder<?,?> createBuilderFromFieldType(final ParseContext context, MappedFieldType fieldType, String currentFieldName) { | |||
| Mapper.Builder builder = null; | |||
| if (fieldType instanceof StringFieldType) { | |||
There was a problem hiding this comment.
This change also makes sense to me, but I'd like @jpountz to take a look at this because I wonder what we might have missed here in terms of tests and also to determine if we should backport this change to other branches. It looks like we never reached the TextFieldType-case in the past and I'm wondering what that means and if it is harmless on the long run.
There was a problem hiding this comment.
Sure, I have amended my commit here. I will open another PR with just this change.
There was a problem hiding this comment.
I just had a chat with @jimczi about the third change in DocumentParser and though it seems like something we need to change indeed, its not immediately clear what side effects this might have, so we'd like to zoom in on that in a bit more detail. Could you open a separate PR for only that change and remove it from this PR, then I can merge the other two changes in more quickly.
* CustomFieldQuery: this was added in: https://lgtm.com/projects/g/elastic/elasticsearch/rev/4756c9a884f3e5341db0cf7799e7e8656c7338d0 but there was already a type check higher up in the same if/else chain. I opted to keep the new one, as it contains a null check that the earlier one does not. * PrioritizedEsThreadPoolExecutor: This check was simply a duplicate of one earlier and would never have been true. The behaviour would have been the same if it was reached.
cbuescher
added
:Internal
>enhancement
v6.2.0
v7.0.0
>non-issue
and removed
>enhancement
labels
There was a problem hiding this comment.
@rneatherway thanks, I will merge the first two changes after the test run has finished.
|
@elasticmachine test this please |
* CustomFieldQuery: removed a redundant type check that was already done higher up in the same if/else chain. * PrioritizedEsThreadPoolExecutor: removed a check that was simply a duplicate of one earlier one and would never have been true.
|
@rneatherway thanks a lot, I merged the first two changes in, the other will be reviewed in #27726. |
|
Cool, happy to help! |
https://lgtm.com/projects/g/elastic/elasticsearch/rev/4756c9a884f3e5341db0cf7799e7e8656c7338d0
but there was already a type check higher up in the same if/else
chain. I opted to keep the new one, as it contains a null check that
the earlier one does not.
of one earlier and would never have been true. The behaviour would
have been the same if it was reached.
earlier check for String, which they are child classes of. I changed
the order to have the more specific checks first so that they are
all now possible.