Skip to content

Deprecate global_ordinals_hash and global_ordinals_low_cardinality #26173

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

Merged
merged 3 commits into from
Aug 21, 2017

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Aug 11, 2017

This change deprecates the global_ordinals_hash and global_ordinals_low_cardinality and
makes the global_ordinals execution hint choose internally if global ords should be remapped or use the segment ord directly.
These hints are too sensitive and expert to be exposed and we should be able to take the right decision internally based on the agg tree.

Relates #26014

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.

I have mixed feelings about this PR. On the one hand I like it because I'd like users to never have to care about the execution mode, but on the other hand this PR moves part of the decision process from the factory to the aggregator, which feels wrong to me, and reduces testing coverage since we cannot force some execution modes anymore.

deprecationLogger.deprecated("global_ordinals_hash is deprecated. Please use [global_ordinals] instead.");
return GLOBAL_ORDINALS;
} else if ("map".equals(value)) {
return MAP;
Copy link
Contributor

Choose a reason for hiding this comment

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

let's do a switch/case?

@jimczi
Copy link
Contributor Author

jimczi commented Aug 14, 2017

but on the other hand this PR moves part of the decision process from the factory to the aggregator, which feels wrong to me, and reduces testing coverage since we cannot force some execution modes anymore.

isn't it the opposite ? The factory now picks global ordinals execution with remapping or plain ordinals with the same heuristic than before. I removed the late decision in the aggregator and replaced it with #26014. The aggregator is just able to remap or not based on the decision made in the factory.
For testing coverage I can add tests for the GlobalOrdinalsStringTermsAggregator which lacks testing anyway ?

@jpountz
Copy link
Contributor

jpountz commented Aug 14, 2017

Woops, I got a bit lost in the diff and misread your PR, sorry. This change is actually great the way it is. Thanks!

+1 to add tests to GlobalOrdinalsStringTermsAggregator.

jimczi added 3 commits August 21, 2017 16:33
This change deprecates the `global_ordinals_hash` and `global_ordinals_low_cardinality` and
makes the `global_ordinals` execution hint choose internally if global ords should be remapped or use the segment ord directly.
These hints are too sensitive and expert to be exposed and we should be able to take the right decision internally based on the agg tree.
@jimczi jimczi force-pushed the global_ordinals_hint branch from 91a37a9 to 96becf6 Compare August 21, 2017 16:58
@jimczi jimczi merged commit 977dcfe into elastic:master Aug 21, 2017
@jimczi jimczi deleted the global_ordinals_hint branch August 21, 2017 17:12
jimczi added a commit that referenced this pull request Aug 21, 2017
jimczi added a commit that referenced this pull request Aug 21, 2017
…26173)

* Deprecate global_ordinals_hash and global_ordinals_low_cardinality

This change deprecates the `global_ordinals_hash` and `global_ordinals_low_cardinality` and
makes the `global_ordinals` execution hint choose internally if global ords should be remapped or use the segment ord directly.
These hints are too sensitive and expert to be exposed and we should be able to take the right decision internally based on the agg tree.
jimczi added a commit that referenced this pull request Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants