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

Exists queries to MatchNoneQueryBuilder when the field is unmapped #54857

Merged
merged 13 commits into from
Apr 27, 2020

Conversation

SivagurunathanV
Copy link
Contributor

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@SivagurunathanV
Copy link
Contributor Author

@jpountz Updated the PR.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I left some suggestions to make it better.

@SivagurunathanV
Copy link
Contributor Author

I updated the PR, let me know if this works.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks close to me, I only left minor comments. Can you also add a testMustRewrite test to ExistsQueryBuilderTests for unmapped fields? You can look at TermQueryBuilderTests for inspiration, I believe that is has such logic.

return Collections.emptySet();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this entire if statement can be removed without affecting correctness. It is not possible to have a field that is neither and object nor a field at this stage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this, if statement we are checking the field type.
MappedFieldType fieldType = context.getMapperService().fieldType(field);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I had the wrong assumption that simpleMatchToIndexName would return an empty set if the field is not mapped.

@SivagurunathanV
Copy link
Contributor Author

Updated the PR with tests.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elasticmachine test this please

return Collections.emptySet();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks I had the wrong assumption that simpleMatchToIndexName would return an empty set if the field is not mapped.

@jpountz
Copy link
Contributor

jpountz commented Apr 16, 2020

@elasticmachine test this please

@SivagurunathanV
Copy link
Contributor Author

SivagurunathanV commented Apr 16, 2020

@jpountz Merged lastest master. Please trigger the test?

@SivagurunathanV
Copy link
Contributor Author

@jpountz any update?

@jpountz
Copy link
Contributor

jpountz commented Apr 18, 2020

@elasticmachine test this please

@SivagurunathanV
Copy link
Contributor Author

@jpountz I didn't find any test failure related to my changes. Am I missing something here? Thanks

@jpountz
Copy link
Contributor

jpountz commented Apr 21, 2020

@elasticmachine update master

@jpountz
Copy link
Contributor

jpountz commented Apr 21, 2020

@elasticmachine test this please

@jpountz
Copy link
Contributor

jpountz commented Apr 21, 2020

@SivagurunathanV Agreed, this looks like a transient failure. I triggered a new build.

@jpountz
Copy link
Contributor

jpountz commented Apr 21, 2020

@SivagurunathanV Can you merge master again?

@SivagurunathanV
Copy link
Contributor Author

@jpountz I merged the latest master. Please trigger a new build to check.

@jpountz
Copy link
Contributor

jpountz commented Apr 21, 2020

@elasticmachine test this please

@SivagurunathanV
Copy link
Contributor Author

SivagurunathanV commented Apr 21, 2020

@jpountz passed 🎉

@SivagurunathanV
Copy link
Contributor Author

SivagurunathanV commented Apr 26, 2020

@jpountz Can you please merge this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants