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

Ignore null value for range field (#27845) #28116

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

liketic
Copy link

@liketic liketic commented Jan 6, 2018

Closes #27845

@elasticmachine
Copy link
Collaborator

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?

1 similar comment
@elasticmachine
Copy link
Collaborator

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?

@cbuescher cbuescher self-assigned this Jan 8, 2018
@cbuescher
Copy link
Member

@elasticmachine test this please

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@liketic thanks a lot, the fix looks good to me at first glance. However, I'd prefer to add unit test to the rest test you added since they are much more controllable. I looked at RangeFieldMapperTests and the tests doTestNullValue() seem to be a good place to extend to add the case where there is no json object at all following the field name. Could you take a look and try to extend that test instead of adding another case to the rest tests? Let me know if you have questions.

@liketic
Copy link
Author

liketic commented Jan 9, 2018

Thanks @cbuescher

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@liketic thanks for adding the test, LGTM.

@cbuescher cbuescher added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types review v7.0.0 v6.3.0 v6.2.0 labels Jan 10, 2018
@cbuescher
Copy link
Member

@elasticmachine test this please

@cbuescher cbuescher merged commit 6c96337 into elastic:master Jan 10, 2018
cbuescher pushed a commit that referenced this pull request Jan 10, 2018
Currently when adding a document with a `null` value for a range field,
the range field mapper raises an error. Instead we should ignore null like 
we do eg. with numbers or geo points.

Closes #27845
@cbuescher
Copy link
Member

cbuescher commented Jan 10, 2018

@liketic thanks, I merged this to master and the 6.x branches, so it will be included in 6.2.

I took the liberty to add a slightly more informative commit message, hope you don't mind.
It would be great if you could add a slightly more descriptive initial comment in future PRs,
since those will end up as commit messages. Ideally the comment includes a more verbose
message explaining the reasoning behind the change. You can find some more thorough
examples here. This helps us reviewing PRs and later when scanning older commits in the log.

Thanks again for all your contributions, they are greatly appreciated.

@liketic
Copy link
Author

liketic commented Jan 11, 2018

Thanks @cbuescher . It's my miss. I will take more attention on the description. Thanks again.

@liketic liketic deleted the bugfix-issues/27845 branch January 11, 2018 01:35
jasontedor added a commit that referenced this pull request Jan 11, 2018
* master: (43 commits)
  Rename core module to server (#28180)
  upgraded jna from 4.4.0-1 to 4.5.1 (#28183)
  [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads
  Primary send safe commit in file-based recovery (#28038)
  [Docs] Correct response json in rank-eval.asciidoc
  Add scroll parameter to _reindex API (#28041)
  Include all sentences smaller than fragment_size in the unified highlighter (#28132)
  Modifies the JavaAPI docs related to AggregationBuilder
  [Docs] Improvements in script-fields.asciidoc (#28174)
  [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019)
  Ignore null value for range field (#27845) (#28116)
  Fix environment variable substitutions in list setting (#28106)
  docs: Replaces indexed script java api docs with stored script api docs
  test: ensure we endup with a single segment
  Make sure that we don't detect files as maven coordinate when installing a plugin (#28163)
  [Tests] temporary disable meta plugin rest tests #28163
  meta-plugin should install bin and config at the top level (#28162)
  Painless: Add public member read/write access test. (#28156)
  Docs: Clarify password protection support with keystore (#28157)
  [Docs] fix plugin properties inclusion for plugins authors
  ...
jasontedor added a commit that referenced this pull request Jan 11, 2018
* 6.x: (41 commits)
  Rename core module to server (#28190)
  [Test] Remove superfluous lines
  [Test] Add skip test for index range field with null values
  upgraded jna from 4.4.0-1 to 4.5.1 (#28183)
  [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads
  [Docs] Correct response json in rank-eval.asciidoc
  Fix environment variable substitutions in list setting (#28106)
  Add scroll parameter to _reindex API (#28041)
  Include all sentences smaller than fragment_size in the unified highlighter (#28132)
  [Docs] Improvements in script-fields.asciidoc (#28174)
  [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019)
  Ignore null value for range field (#27845) (#28116)
  docs: Replaces indexed script java api docs with stored script api docs
  test: ensure we endup with a single segment
  Make sure that we don't detect files as maven coordinate when installing a plugin (#28163)
  [Tests] temporary disable meta plugin rest tests #28163
  meta-plugin should install bin and config at the top level (#28162)
  Painless: Add public member read/write access test. (#28156)
  Docs: Clarify password protection support with keystore (#28157)
  [Docs] fix plugin properties inclusion for plugins authors
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 13, 2018
* master: (30 commits)
  Fix lock accounting in releasable lock
  Add ability to associate an ID with tasks  (elastic#27764)
  [DOCS] Removed differencies between text and code (elastic#27993)
  text fixes (elastic#28136)
  Update getting-started.asciidoc (elastic#28145)
  [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187)
  Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186)
  Do not keep 5.x commits once having 6.x commits (elastic#28188)
  Rename core module to server (elastic#28180)
  upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183)
  [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads
  Primary send safe commit in file-based recovery (elastic#28038)
  [Docs] Correct response json in rank-eval.asciidoc
  Add scroll parameter to _reindex API (elastic#28041)
  Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132)
  Modifies the JavaAPI docs related to AggregationBuilder
  [Docs] Improvements in script-fields.asciidoc (elastic#28174)
  [Docs] Remove Kerberos/SPNEGO Shield plugin (elastic#28019)
  Ignore null value for range field (elastic#27845) (elastic#28116)
  Fix environment variable substitutions in list setting (elastic#28106)
  ...
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v6.2.0 v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants