Skip to content

Use global_ordinals_hash execution mode when sorting by sub aggregations.#26014

Merged
jpountz merged 1 commit intoelastic:masterfrom
jpountz:fix/hash_ords_when_sorting_by_sub_agg
Aug 10, 2017
Merged

Use global_ordinals_hash execution mode when sorting by sub aggregations.#26014
jpountz merged 1 commit intoelastic:masterfrom
jpountz:fix/hash_ords_when_sorting_by_sub_agg

Conversation

@jpountz
Copy link
Copy Markdown
Contributor

@jpountz jpountz commented Aug 2, 2017

This is a safer default since sorting by sub aggregations prevents these
aggregations from being deferred. global_ordinals_hash will at least
make sure that we do not use memory for buckets that are not collected.

Closes #24359

@jpountz
Use global_ordinals_hash execution mode when sorting by sub aggrega…
906f3a4 
…tions.

This is a safer default since sorting by sub aggregations prevents these
aggregations from being deferred. `global_ordinals_hash` will at least
make sure that we do not use memory for buckets that are not collected.

Closes elastic#24359
martijnvg
martijnvg approved these changes Aug 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

@jpountz jpountz merged commit 0bf8a35 into elastic:master Aug 10, 2017
@jpountz jpountz deleted the fix/hash_ords_when_sorting_by_sub_agg branch August 10, 2017 10:28
jpountz added a commit that referenced this pull request Aug 10, 2017
@jpountz
Use global_ordinals_hash execution mode when sorting by sub aggrega…
e565840 
…tions. (#26014)

This is a safer default since sorting by sub aggregations prevents these
aggregations from being deferred. `global_ordinals_hash` will at least
make sure that we do not use memory for buckets that are not collected.

Closes #24359
@colings86
Copy link
Copy Markdown
Contributor

colings86 commented Aug 11, 2017

Could this be back-ported to the 6.0 branch? Although this is labelled as an enhancement I think it will help to prevent OOMEs during the collect phases of aggregations especially when sorting by the cardinality agg which seems to be relatively popular.

@jpountz
Copy link
Copy Markdown
Contributor Author

jpountz commented Aug 11, 2017

Agreed, I'll backport!

jpountz added a commit that referenced this pull request Aug 11, 2017
@jpountz
Use global_ordinals_hash execution mode when sorting by sub aggrega…
10e3a99 
…tions. (#26014)

This is a safer default since sorting by sub aggregations prevents these
aggregations from being deferred. `global_ordinals_hash` will at least
make sure that we do not use memory for buckets that are not collected.

Closes #24359
@jpountz
Copy link
Copy Markdown
Contributor Author

jpountz commented Aug 11, 2017

Done.

@jimczi
Copy link
Copy Markdown
Contributor

jimczi commented Aug 11, 2017

Sorry I missed this pr, a broader protection has been added with
#24941 which forces the remap of global ordinals when a sub-aggregator cannot be defered even when the global_ordinals mode is used.
These execution hint are really sensitive so I tried to make global_ordinals less trappy but I see that it can also be confusing to still have the distinction.
We can maybe deprecate global_ordinals_hash to make it clear that global_ordinals mode will choose the best option to collect sub aggregations ?

@jpountz
Copy link
Copy Markdown
Contributor Author

jpountz commented Aug 11, 2017

I think that makes sense. We'd need to apply the other heuristics that we have in the factory such as whether there is a parent bucket aggregation. But then I'm a bit annoyed by the fact that we'd be making most decisions at the Aggregator level while it really belongs to the factory. Not sure, maybe you have opinions/ideas around that?

@jimczi jimczi mentioned this pull request Aug 11, 2017
Merged
@jimczi
Copy link
Copy Markdown
Contributor

jimczi commented Aug 11, 2017

I opened #26173 which deprecates global_ordinals_hash and global_ordinals_low_cardinality and moves all the decisions regarding remapping global ords and low cardinality to the factory.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2017
@jasontedor
Merge branch 'master' into es-path-conf
b901de5 
* master: (30 commits)
  Rewrite range queries with open bounds to exists query (elastic#26160)
  Fix eclipse compilation problem (elastic#26170)
  Epoch millis and second formats parse float implicitly (Closes elastic#14641) (elastic#26119)
  fix SplitProcessor targetField test (elastic#26178)
  Fixed typo in README.textile (elastic#26168)
  Fix incorrect class name in deleteByQuery docs (elastic#26151)
  Move more token filters to analysis-common module
  reindex: automatically choose the number of slices (elastic#26030)
  Fix serialization of the `_all` field. (elastic#26143)
  percolator: Hint what clauses are important in a conjunction query based on fields
  Remove unused Netty-related settings (elastic#26161)
  Remove SimpleQueryStringIT#testPhraseQueryOnFieldWithNoPositions.
  Tests: reenable ShardReduceIT#testIpRange.
  Allow `ClusterState.Custom` to be created on initial cluster states (elastic#26144)
  Teach the build about betas and rcs (elastic#26066)
  Fix wrong header level
  inner hits: Unfiltered nested source should keep its full path
  Document how to import Lucene Snapshot libs when elasticsearch clients (elastic#26113)
  Use `global_ordinals_hash` execution mode when sorting by sub aggregations. (elastic#26014)
  Make the README use a single type in examples. (elastic#26098)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martijnvg martijnvg martijnvg approved these changes

Assignees

No one assigned

Labels

:Analytics/Aggregations Aggregations >enhancement v6.0.0-beta2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jpountz @colings86 @jimczi @martijnvg