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.

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

Aggregations/HL Rest client fix: missing scores #32774

Merged
merged 3 commits into from
Aug 14, 2018
Merged

Aggregations/HL Rest client fix: missing scores #32774

merged 3 commits into from
Aug 14, 2018

Conversation

markharwood
Copy link
Contributor

Significant_terms' score doubles were being parsed as long.
Added test.
Thanks for the report/fix, Blakko

Closes #32770

@markharwood
Aggregations/HL Rest client fix: significance score doubles were bein…
a5dbe99 
…g parsed as long.

Added test.
Thanks for the report/fix, Blakko

Closes elastic#32770
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

colings86
colings86 approved these changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM when the build passes. I think this will need backporting to 6.x. I don't think there is a bwc issue here, this is clearly a bug fix which stops the API from working and since JSON the score field in the bucket is a double type I don't think this breaks bwc anyway

@markharwood
Copy link
Contributor Author

test this please

@markharwood
Copy link
Contributor Author

run gradle build tests

javanna
javanna reviewed Aug 10, 2018
View reviewed changes
...in/java/org/elasticsearch/search/aggregations/bucket/significant/ParsedSignificantTerms.java
@@ -175,7 +175,7 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
bucket.subsetDf = value;
bucket.setDocCount(value);
} else if (InternalSignificantTerms.SCORE.equals(currentFieldName)) {
bucket.score = parser.longValue();
bucket.score = parser.doubleValue();
Copy link
Member

Choose a reason for hiding this comment

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

ops :)

Copy link
Member

Choose a reason for hiding this comment

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

would it maybe be possible to recreate this in a unit test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any examples of existing unit tests for "toXContent -> fromXContent" serialization on aggregation buckets I can base this on?

Copy link
Member

Choose a reason for hiding this comment

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

as far as I can see BaseAggregationTestCase has a testFromXContent method. So SignficantTermsTests sounds like the appropriate test.

Copy link
Contributor Author

@markharwood markharwood Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

BaseAggregationTestCase has a testFromXContent method

In which case we might be missing some test infrastructure for aggs here. That method is for testing agg requests but we have no equivalent for testing responses. I guess toXcontent->fromXContent is a transformation of response objects that didn't exist before high level rest client.

Copy link
Member

Choose a reason for hiding this comment

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

Right, that is for requests, my bad, but we have tests for responses too, for instance InternalTermsTestCase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a quick chat with @colings86 and it looks like we need a base class for aggs with an abstract assertParsedResponse(InternalX, ParsedX). InternalX here is the object used to hold the response on the server and ParsedX is the client-side equivalent.

That work's beyond the scope of this fix so I suggest we open another issue to address that broader set of work and put this PR to bed as-is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just looked and actually InternalAggregationTestCase already has a method of ensuringthe serialisation is correct in it's testFromXContent() method. So I think this might be a bug in InternalSignificantTermsTestCase which is not producing random instance that exercise this. We can however produce a new test method to make sure this is tested using parseAndAssert()andassertFromXContent()` even if we can't make the randomised test instances exercise the code path directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, @colings86. The buckets need updateScore calling on them in the test code to derive scores from all the frequency stats. Otherwise tests were always testing for score=0 conditions and failing to highlight the bug.
When I remove fromXContent fix and run with the beefed up tests it now reproduces the original error so it looks like the test infra is there.

Copy link
Member

Choose a reason for hiding this comment

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

glad to hear, am a bit rusty on this topic but I kind of knew/hoped that we have added tests when adding parsing for the high-level REST client.

@markharwood
Copy link
Contributor Author

Good to go on this, @colings86 @javanna ?

colings86
colings86 approved these changes Aug 14, 2018
View reviewed changes
Copy link
Contributor

@colings86 colings86 left a comment

Choose a reason for hiding this comment

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

LGTM if @javanna is happy too

javanna
javanna approved these changes Aug 14, 2018
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM thanks @markharwood & @colings86 !!!

@markharwood markharwood merged commit e5ab09f into elastic:master Aug 14, 2018
markharwood added a commit that referenced this pull request Aug 14, 2018
@markharwood
Aggregations/HL Rest client fix: missing scores (#32774)
a25f1ab 
Significance score doubles were being parsed as long. Existing tests did not catch this because SignificantLongTermsTests and SignificantStringTermsTests did not set the score. Fixed these and also added integration test.

Thanks for the report/fix, Blakko

Closes #32770
@andyb-elastic andyb-elastic mentioned this pull request Aug 14, 2018
Closed
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into allowed-index-stor…
ec4ab87 
…e-types

* elastic/master: (199 commits)
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into global-checkpoint-…
aad650a 
…listeners

* elastic/master:
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
jasontedor added a commit that referenced this pull request Aug 15, 2018
@jasontedor
Merge branch '6.x' into ccr-6.x
53cee21 
* 6.x: (96 commits)
  Introduce global checkpoint listeners (#32696)
  Use JDK 10 for 6.4 BWC builds (#32866)
  Remove unused imports - follow up to removal of test in issue 32855
  Removed flaky test. Looks like randomisation makes these assertions unreliable. This test is superfluous - it was added to address #32770 but it later turned out there was an existing test that just required a fix to provide the missing test coverage.
  [test] mute IndexShardTests.testDocStats
  Test: Fix forbidden uses in test framework (#32824)
  Security: remove password hash bootstrap check (#32440)
  Move validation to server for put user requests (#32471)
  [ML] Add high level REST client docs for ML put job endpoint (#32843)
  Painless: Change fqn_only to no_import (#32817)
  [test] mute testSearchWithSignificantTermsAgg
  Backport: CompletableContext class to avoid throwable (#32829)
  [TEST] Select free port for Minio (#32837)
  SCRIPTING: Support BucketAggScript return null (#32811) (#32833)
  HLRC: Add Delete License API (#32586)
  Aggregations/HL Rest client fix: missing scores (#32774)
  HLRC: migration get assistance API (#32744)
  Fix NOOP bulk updates (#32819)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  ...
@colings86 colings86 removed the :Analytics/Aggregations Aggregations label Nov 2, 2018
@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
Reviewers

@colings86 colings86 colings86 approved these changes

@javanna javanna javanna approved these changes

Assignees
No one assigned
Labels
>bug v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Truncated score when using the significat terms aggregation
5 participants
@markharwood @elasticmachine @colings86 @javanna @jimczi