Skip to content

Remove UnsortedNumericDoubleValues #26817

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 6, 2017
Merged

Remove UnsortedNumericDoubleValues #26817

merged 2 commits into from
Oct 6, 2017

Conversation

liketic
Copy link

@liketic liketic commented Sep 28, 2017

Resolves #24086

@jpountz Thanks for your patient for this fix. There still a test failed because the test expects the nextValue() of NumericDoubleValues can be called several times. But the SortingNumericDoubleValues cannot be reused. So I need to fix the test or allow reset the SortingNumericDoubleValues's state? Thanks.

@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?

@jpountz
Copy link
Contributor

jpountz commented Sep 28, 2017

@liketic what is the failing test? maybe the bug is in the test?

@liketic
Copy link
Author

liketic commented Sep 29, 2017

@jpountz I found the root cause is doubleValue() was called twice after called advanceExact(). I already fixed it, could you help to review this. Thanks very much!

@liketic
Copy link
Author

liketic commented Oct 5, 2017

@jpountz Could you please help review this PR if you're available? 😅

@jpountz
Copy link
Contributor

jpountz commented Oct 5, 2017

Sorry I had other priorities over the past week. I should have time to give it a look tomorrow if not today.

jpountz
jpountz approved these changes Oct 6, 2017
View reviewed changes
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.

Your changes look good, thanks! I'll make them run through CI.

However I think you found a bug: it should be legal to call doubleValue() twice. Have you already looked into what impl had the issue?

@jpountz
Copy link
Contributor

jpountz commented Oct 6, 2017

@elasticmachine please test it

@liketic
Copy link
Author

liketic commented Oct 6, 2017 •
edited
Loading

@jpountz From now I only sure SortingNumericDoubleValues has this problem, because a SortingNumericDoubleValues instance will keep all values in a internal array, and use a cursor to access elements in this array. Which means after the cursor exceeds the size of array, a error wil thrown. And if a NumericDoubleValues wrapped a SortingNumericDoubleValues, then the doubleValue() of NumericDoubleValues cannot called more than once bacause we have no api to reset the SortingNumericDoubleValues's cursor.

Seems like the testing is failed, can I rebase it? Thanks for reviewing this PR.

@jpountz
Copy link
Contributor

jpountz commented Oct 6, 2017

I see. So we should change those wrappers to cache the value when advanceExact is called.

@jpountz jpountz merged commit 100e3c9 into elastic:master Oct 6, 2017
jpountz pushed a commit that referenced this pull request Oct 6, 2017
@jpountz
Remove UnsortedNumericDoubleValues (#26817)
25cf81b 
Closes #24086
@jpountz jpountz added :Search/Search Search-related issues that do not fall into other categories >non-issue v6.1.0 v7.0.0 labels Oct 6, 2017
@liketic liketic deleted the issues/24086 branch October 6, 2017 15:58
@liketic
Copy link
Author

liketic commented Oct 6, 2017

@jpountz If you don't mind I can send another PR with that fix.

@jpountz
Copy link
Contributor

jpountz commented Oct 6, 2017

Please do! :)

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 6, 2017
@jasontedor
Merge branch 'ccr' into engine-factory-provider
5506cfd 
* ccr: (42 commits)
  [DOCS] Added info about snapshotting your data before an upgrade.
  Add documentation about disabling `_field_names`. (elastic#26813)
  Remove UnsortedNumericDoubleValues (elastic#26817)
  Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856)
  [TEST] Added skipping the `headers` feature to the Bulk REST YAML test
  Update type-field.asciidoc
  Fix search_after with geo distance sorting (elastic#26891)
  Use proper logging placeholder for Netty logging
  Add Netty channel information on write and flush failure
  Remove deploying in JBoss documentation
  Document JVM option MaxFDLimit for macOS ()
  Add additional low-level logging handler ()
  Unwrap causes when maybe dying
  Change log level on write and flush failure to warn
  [TEST] add test to ensure legacy list syntax in yml works fine
  Bump BWC version for settings serialization to 6.1.0
  Removed void token filter entries and added two tests
  Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238)
  Fix toString() in SnapshotStatus (elastic#26852)
  elastic#26870 change bwc version for fuzzy_transpositions to 6.1 after backport
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Oct 6, 2017
@jasontedor
Merge branch 'ccr' into newSnapshotBetweenMinAndMaxSeqNo
2e27e0b 
* ccr: (110 commits)
  Use LF line endings in Painless generated files (elastic#26822)
  [DOCS] Added info about snapshotting your data before an upgrade.
  Add documentation about disabling `_field_names`. (elastic#26813)
  Remove UnsortedNumericDoubleValues (elastic#26817)
  Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856)
  [TEST] Added skipping the `headers` feature to the Bulk REST YAML test
  Update type-field.asciidoc
  Fix search_after with geo distance sorting (elastic#26891)
  Use proper logging placeholder for Netty logging
  Add Netty channel information on write and flush failure
  Remove deploying in JBoss documentation
  Document JVM option MaxFDLimit for macOS ()
  Add additional low-level logging handler ()
  Unwrap causes when maybe dying
  Change log level on write and flush failure to warn
  [TEST] add test to ensure legacy list syntax in yml works fine
  Bump BWC version for settings serialization to 6.1.0
  Removed void token filter entries and added two tests
  Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238)
  Fix toString() in SnapshotStatus (elastic#26852)
  ...
@liketic liketic mentioned this pull request Oct 7, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove UnsortedNumericDoubleValues
4 participants
@liketic @elasticmachine @jpountz @colings86