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

Upgrade to latest Lucene snapshot #33505

Merged
merged 10 commits into from
Sep 10, 2018
Merged

Upgrade to latest Lucene snapshot #33505

merged 10 commits into from
Sep 10, 2018

Conversation

romseygeek
Copy link
Contributor

  • Collectors now take Scorable
  • Adds IndexWriter.getFlushingBytes()

@romseygeek romseygeek self-assigned this Sep 7, 2018
@romseygeek romseygeek added >non-issue :Search/Search Search-related issues that do not fall into other categories v7.0.0 labels Sep 7, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

// when minScore is set, scores might be requested twice: once
// to verify the match, and once by the collector
scorer = new ScoreCachingWrappingScorer(scorer);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ScoreCachingWrapperScorer is now a Scorable, so this no longer works. Instead I've just duplicated the caching logic here. Given that SCWS is only ever applied by Collectors, and this is a real Scorer (so won't be generated by a Collector), we don't need to worry about double wrapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

you probably need to fix the two-phase iterator to call this.score() rather than in.score() as well then, or otherwise it by-passes caching?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe the two-phase iterator could just do curScore = in.score(); return curScore >= minScore; and then score() wouldn't even have to check the current doc ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

I also changed MinScoreScorerTests to check that score() only gets called once on the wrapped Scorable.

@jpountz jpountz changed the title Upgrade to latest snapshot Upgrade to latest Lucene snapshot Sep 10, 2018
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.

One minor problem to address, otherwise LGTM!

// when minScore is set, scores might be requested twice: once
// to verify the match, and once by the collector
scorer = new ScoreCachingWrappingScorer(scorer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably need to fix the two-phase iterator to call this.score() rather than in.score() as well then, or otherwise it by-passes caching?

// when minScore is set, scores might be requested twice: once
// to verify the match, and once by the collector
scorer = new ScoreCachingWrappingScorer(scorer);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe the two-phase iterator could just do curScore = in.score(); return curScore >= minScore; and then score() wouldn't even have to check the current doc ID

@romseygeek romseygeek merged commit 39c3234 into elastic:master Sep 10, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 11, 2018
* master:
  Add full cluster restart base class (elastic#33577)
  Validate list values for settings (elastic#33503)
  Copy and validatie soft-deletes setting on resize (elastic#33517)
  Test: Fix package name
  SQL: Fix result column names for arithmetic functions (elastic#33500)
  Upgrade to latest Lucene snapshot (elastic#33505)
  Enable not wiping cluster settings after REST test (elastic#33575)
  MINOR: Remove Dead Code in SearchScript (elastic#33569)
  [Test] Remove duplicate method in TestShardRouting (elastic#32815)
  mute test on windows
  Update beats template to include apm-server metrics (elastic#33286)
  Fix typos (elastic#33499)
  [CCR] Delay auto follow license check (elastic#33557)
  [CCR] Add create_follow_index privilege (elastic#33559)
  Strengthen FilterRoutingTests (elastic#33149)
  Correctly handle PKCS#11 tokens for system keystore (elastic#33460)
  Remove some duplicate request conversion methods. (elastic#33538)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 11, 2018
* master: (91 commits)
  Preserve cluster settings on full restart tests (elastic#33590)
  Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582)
  Fix upgrading of list settings (elastic#33589)
  Add read-only Engine (elastic#33563)
  HLRC: Add ML get categories API (elastic#33465)
  SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411)
  Add predicate_token_filter (elastic#33431)
  Fix Replace function. Adds more tests to all string functions. (elastic#33478)
  [ML] Rename input_fields to column_names in file structure (elastic#33568)
  Add full cluster restart base class (elastic#33577)
  Validate list values for settings (elastic#33503)
  Copy and validatie soft-deletes setting on resize (elastic#33517)
  Test: Fix package name
  SQL: Fix result column names for arithmetic functions (elastic#33500)
  Upgrade to latest Lucene snapshot (elastic#33505)
  Enable not wiping cluster settings after REST test (elastic#33575)
  MINOR: Remove Dead Code in SearchScript (elastic#33569)
  [Test] Remove duplicate method in TestShardRouting (elastic#32815)
  mute test on windows
  Update beats template to include apm-server metrics (elastic#33286)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants