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

Forbid negative weight in Function Score Query #33390

Merged
merged 5 commits into from
Sep 12, 2018
Merged

Forbid negative weight in Function Score Query #33390

merged 5 commits into from
Sep 12, 2018

Conversation

lipsill
Copy link
Contributor

@lipsill lipsill commented Sep 4, 2018

Relates to #31927

@colings86 colings86 added the :Search/Search Search-related issues that do not fall into other categories label Sep 5, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

jimczi
jimczi requested changes Sep 7, 2018
View reviewed changes
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.

Thanks @lipsill ! The changes look good, I left a small comment regarding the check for negative values.

server/src/main/java/org/elasticsearch/index/query/functionscore/ScoreFunctionBuilder.java Outdated
return (FB) this;
}

private Float checkWeight(Float weight) {
if (weight != null && weight < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do Float.compare(weight, 0) < 0)

jimczi
jimczi requested changes Sep 7, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/index/query/functionscore/ScoreFunctionBuilder.java Outdated
return (FB) this;
}

private Float checkWeight(Float weight) {
if (weight != null && weight < 0) {
DEPRECATION_LOGGER.deprecated("[weight] cannot be negative for a filtering function [{}]", weight);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is master can you throw an IllegalArgumentException and we'll handle the deprecation when we backport to 6x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... I thought that it is preferred to have one PR against master handing the deprecation (that is backported to 6.x) and a second (followup) PR for the removal (and IAE) that is only on master.
My bad;) I will change the PR shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this one I think we can have two prs, one for the removal and one for the deprecation that targets 6x only. There are some [tests]6269744#diff-9af0a85c71293ca3d30f83fe80aedd28 in master that will fail in 6x because they produce negative weights so the backport is not trivial.

@jimczi jimczi changed the title Deprecation message when the weight is negative in a Function Score Query Forbid negative weight in Function Score Query Sep 11, 2018
jimczi
jimczi reviewed Sep 11, 2018
View reviewed changes
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.

Thanks @lipsill ! It looks good to me. I'll merge if the CI passes.

@jimczi
Copy link
Contributor

jimczi commented Sep 11, 2018

test this please

@lipsill
Copy link
Contributor Author

lipsill commented Sep 11, 2018

@jimczi the CI build failed with listener timeout after waiting for [30000] ms. I doubt this is caused by the PR so I just merged master again ;)

@jimczi
Copy link
Contributor

jimczi commented Sep 11, 2018

test this please

@jimczi jimczi merged commit c92ec1c into elastic:master Sep 12, 2018
@jimczi
Copy link
Contributor

jimczi commented Sep 12, 2018

Thanks @lipsill ! As discussed earlier we need a new pr for the backport in 6x where some tests need to be adapted.

@lipsill lipsill deleted the neg_weight_depr branch September 12, 2018 09:58
@lipsill
Copy link
Contributor Author

lipsill commented Sep 12, 2018

@jimczi thanks!

As discussed earlier we need a new pr for the backport in 6x where some tests need to be adapted.

I just opened #33624 against 6.x.

jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 12, 2018
@jasontedor
Merge branch 'master' into pr/33501
2c3a276 
* master: (43 commits)
  [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603)
  Update AWS SDK to 1.11.406  in repository-s3 (elastic#30723)
  Expose CCR stats to monitoring (elastic#33617)
  [Docs] Update match-query.asciidoc (elastic#33610)
  TEST: Adjust rollback condition when shard is empty
  [CCR] Improve shard follow task's retryable error handling (elastic#33371)
  Forbid negative `weight` in Function Score Query (elastic#33390)
  Clarify context suggestions filtering and boosting (elastic#33601)
  Disable CCR REST endpoints if CCR disabled (elastic#33619)
  Lower version on full cluster restart settings test
  Upgrade remote cluster settings (elastic#33537)
  NETWORKING: http.publish_host Should Contain CNAME (elastic#32806)
  Add test coverage for global checkpoint listeners
  Reset replica engine to global checkpoint on promotion (elastic#33473)
  HLRC: ML Delete Forecast API (elastic#33526)
  Remove debug logging in full cluster restart tests (elastic#33612)
  Expose CCR to the transport client (elastic#33608)
  Mute testIndexDeletionWhenNodeRejoins
  SQL: Make Literal a NamedExpression (elastic#33583)
  [DOCS] Adds missing built-in user information (elastic#33585)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 12, 2018
@jasontedor
Merge branch 'master' into pr/33367
310ca5d 
* master: (128 commits)
  [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603)
  Update AWS SDK to 1.11.406  in repository-s3 (elastic#30723)
  Expose CCR stats to monitoring (elastic#33617)
  [Docs] Update match-query.asciidoc (elastic#33610)
  TEST: Adjust rollback condition when shard is empty
  [CCR] Improve shard follow task's retryable error handling (elastic#33371)
  Forbid negative `weight` in Function Score Query (elastic#33390)
  Clarify context suggestions filtering and boosting (elastic#33601)
  Disable CCR REST endpoints if CCR disabled (elastic#33619)
  Lower version on full cluster restart settings test
  Upgrade remote cluster settings (elastic#33537)
  NETWORKING: http.publish_host Should Contain CNAME (elastic#32806)
  Add test coverage for global checkpoint listeners
  Reset replica engine to global checkpoint on promotion (elastic#33473)
  HLRC: ML Delete Forecast API (elastic#33526)
  Remove debug logging in full cluster restart tests (elastic#33612)
  Expose CCR to the transport client (elastic#33608)
  Mute testIndexDeletionWhenNodeRejoins
  SQL: Make Literal a NamedExpression (elastic#33583)
  [DOCS] Adds missing built-in user information (elastic#33585)
  ...
@jimczi jimczi mentioned this pull request Sep 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimczi jimczi jimczi approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@lipsill @elasticmachine @jimczi @colings86