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

Uses norms for exists query if enabled #27237

Merged
merged 2 commits into from
Nov 3, 2017
Merged

Uses norms for exists query if enabled #27237

merged 2 commits into from
Nov 3, 2017

Conversation

colings86
Copy link
Contributor

This change means that for indexes created from 6.1.0, if normas are enabled we will not write the field name to the _field_names field and for an exists query we will instead use the NormsFieldExistsQuery which was added in Lucene 7.1.0. If norms are not enabled or if the index was created before 6.1.0 _field_names will be used as before.

This change means that for indexes created from 6.1.0, if normas are enabled we will not write the field name to the `_field_names` field and for an exists query we will instead use the NormsFieldExistsQuery which was added in Lucene 7.1.0. If norms are not enabled or if the index was created before 6.1.0 `_field_names` will be used as before.
@colings86 colings86 added :Search/Search Search-related issues that do not fall into other categories >enhancement review v6.1.0 v7.0.0 labels Nov 2, 2017
@colings86 colings86 self-assigned this Nov 2, 2017
@colings86 colings86 requested a review from jpountz November 2, 2017 16:01
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM
What is the behavior when the field is indexed with an explicit null value ? I guess it matches the exists query with the term based query (_field_names field) but not with doc values based ? If it's the case we should probably align them and change the documentation.

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.

The change looks good, let's add a test? Should we also implement it on keywords that have doc values disabled and norms enabled?

@colings86
Copy link
Contributor Author

@jimczi I believe that explicit nulls are treated the same as if the field is missing entirely. I thought this was true of the indexing process in general but maybe @jpountz can confirm?

@jpountz I think we have existing tests that cover this change? There is 160_existsQuery.yml and also the ExistQueryBuilderTests and RangeQueryBuilderTests which both cover the case where the field is of type text? We could implement this on keywords but is it likely that a user would disable doc values but not norms?

@jimczi
Copy link
Contributor

jimczi commented Nov 2, 2017

@jimczi I believe that explicit nulls are treated the same as if the field is missing entirely.

Right, sorry for the confusion. It works the same for both impl so nothing to do here.

@jpountz
Copy link
Contributor

jpountz commented Nov 2, 2017

@jpountz I think we have existing tests that cover this change?

I was confused because I did not see changes to the tests, while I would have expected some tests to be changed to expect a NormFieldExistsQuery. I see them now so I'm good with the tests as they are.

We could implement this on keywords but is it likely that a user would disable doc values but not norms?

This is unlikely indeed. Maybe it is just something to keep in mind if we see such cases in the future?

@colings86 colings86 merged commit 28b4d95 into elastic:master Nov 3, 2017
@colings86
Copy link
Contributor Author

Thanks for the reviews @jpountz @jimczi

colings86 added a commit that referenced this pull request Nov 3, 2017
* Uses norms for exists query if enabled

This change means that for indexes created from 6.1.0, if normas are enabled we will not write the field name to the `_field_names` field and for an exists query we will instead use the NormsFieldExistsQuery which was added in Lucene 7.1.0. If norms are not enabled or if the index was created before 6.1.0 `_field_names` will be used as before.

* Fixes tests
@colings86 colings86 deleted the enhance/text_exists_query branch November 3, 2017 09:06
martijnvg added a commit that referenced this pull request Nov 4, 2017
* es/master:
  Fix snapshot getting stuck in INIT state (#27214)
  Add an example of dynamic field names (#27255)
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix InternalStatsTests
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
martijnvg added a commit that referenced this pull request Nov 4, 2017
* 6.x:
  Add an example of dynamic field names (#27255)
  fixed checkstyle violation
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  [TEST] Fix failing exists query test
  test: Do not run old parent/child tests against a cluster with minimum version greater than 6.0.0
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  fix compil after backport
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
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 v6.1.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants