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

[Docs] Correct default rescore window_size #31582

Merged
merged 1 commit into from
Jul 4, 2018
Merged

[Docs] Correct default rescore window_size #31582

merged 1 commit into from
Jul 4, 2018

Conversation

cbuescher
Copy link
Member

I was looking into some unrelated issue in the query rescorer and was suprised to see that the
default window_size parameter doesn't seem to be related to from/size any more. From looking
at the code and trying it out it seems to always default to 10.

This might be the wrong behaviour, but at least that seems to be the current one so I first want to
suggest making the doc change. I'll do a bit of digging when this might have changed and if it was
a concious decision or somehow slipped in.

@cbuescher cbuescher added >docs General docs changes :Search Relevance/Ranking Scoring, rescoring, rank evaluation. v7.0.0 v6.5.0 labels Jun 26, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@cbuescher cbuescher changed the title [Docs] Correct default window_size [Docs] Correct default rescore window_size Jun 26, 2018
@cbuescher
Copy link
Member Author

Done some digging and at least from the code side it looks like 10 has has been default for a long time (on the 5.x branches at least). Not sure where note of from/size comes from in the docs still.

@jasontedor jasontedor added v6.4.0 and removed v6.5.0 labels Jun 28, 2018
jimczi
jimczi approved these changes Jul 4, 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.

LGTM, the same window_size must be used within a pagination so the default is a constant:

when exposing pagination to your users, you should not change window_size as you step through each page (by passing different from values) since that can alter the top hits causing results to confusingly shift as the user steps through pages.

@jpountz
Copy link
Contributor

jpountz commented Jul 4, 2018

+1 to a constant too

@cbuescher
Copy link
Member Author

@jimczi @jpountz thanks for taking a look

@cbuescher cbuescher merged commit 5f87a84 into elastic:master Jul 4, 2018
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
d646fc4
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
c2d4bec
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
7b75c4e
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
4cc7289
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
40fdc98
cbuescher pushed a commit that referenced this pull request Jul 4, 2018
[Docs] Correct default window_size (#31582)
a2bbbf6
dnhatn added a commit that referenced this pull request Jul 4, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
151d244 
* 6.x:
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758) (#31789)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  [ML] Limit ML filter items to 10K (#31731)
  Fixture for Minio testing (#31688)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Painless: Complete Removal of Painless Type (#31699)
  Consolidate watcher setting update registration (#31762)
  [DOCS] Adds empty 6.3.1 release notes page
  ingest: Introduction of a bytes processor (#31733)
  [test] don't run bats tests for suse boxes (#31749)
  Add analyze API to high-level rest client (#31577)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  Support multiple system store types (#31650)
  Add write*Blob option to replace existing blob (#31729)
  Split CircuitBreaker-related tests (#31659)
  Painless: Add Context Docs (#31190)
  Docs: Remove missing reference
  Migrate scripted metric aggregation scripts to ScriptContext design (#30111)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Remove _all example (#31711)
  rest-high-level: added get cluster settings (#31706)
  Docs: Match the examples in the description (#31710)
  [Docs] Correct typos (#31720)
  Extend allowed characters for grok field names (#21745) (#31653) (#31722)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Fix gradle4.8 deprecation warnings (#31654)
  Update numbers to reflect 4-byte UTF-8-encoded characters (#27083)
dnhatn added a commit that referenced this pull request Jul 4, 2018
@dnhatn
Merge branch 'master' into ccr
f736205 
* master:
  [ML] Rate limit established model memory updates (#31768)
  [Docs] Correct default window_size (#31582)
  S3 fixture should report 404 on unknown bucket (#31782)
  Detach Transport from TransportService (#31727)
  [ML] Limit ML filter items to 10K (#31731)
  [ML] Return statistics about forecasts as part of the jobsstats and usage API (#31647)
  Fixture for Minio testing (#31688)
  [DOCS] Add missing get mappings docs to HLRC (#31765)
  [DOCS] Starting Elasticsearch (#31701)
  Painless: Complete Removal of Painless Type (#31699)
  Fix not waiting for Netty ThreadDeathWatcher in IT (#31758)
  Consolidate watcher setting update registration (#31762)
  Build: re-enabled bwc (#31769)
  ingest: Introduction of a bytes processor (#31733)
  Fix coerce validation_method in GeoBoundingBoxQueryBuilder (#31747)
  Add analyze API to high-level rest client (#31577)
  [DOCS] Typos
  DOC: Add examples to the SQL docs (#31633)
  Add support for AWS session tokens (#30414)
  Watcher: Reenable start/stop yaml tests (#31754)
  Implemented XContent serialisation for GetIndexResponse (#31675)
  JDBC: Fix stackoverflow on getObject and timestamp conversion (#31735)
  resolveHasher defaults to NOOP (#31723)
  Account for XContent overhead in in-flight breaker
  Split CircuitBreaker-related tests (#31659)
  Add write*Blob option to replace existing blob (#31729)
  Painless: Add Context Docs (#31190)
  Watcher: Fix chain input toXcontent serialization (#31721)
  Docs: Match the examples in the description (#31710)
  rest-high-level: added get cluster settings (#31706)
  [Docs] Correct typos (#31720)
  Clean up double semicolon code typos (#31687)
  [DOCS] Check for Windows and *nix file paths (#31648)
  [ML] Validate ML filter_id (#31535)
  Revert long lines
  Fix TransportChangePasswordActionTests
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
>docs General docs changes :Search Relevance/Ranking Scoring, rescoring, rank evaluation. v5.6.11 v6.0.3 v6.1.5 v6.2.5 v6.3.2 v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cbuescher @elasticmachine @jpountz @jimczi @colings86 @jasontedor