Fix time-series geo_line to include reduce phase in MergedGeoLines #96953
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
craigtaverner added >enhancement :Analytics/Geo 
| Pinging @elastic/es-analytics-geo (Team:Analytics) |
| Hi @craigtaverner, I've created a changelog YAML for you. |
20 tasks
craigtaverner force-pushed the merged_geo_lines_simplifier branch from 87361e0 to 383e605 Compare craigtaverner changed the title | Hi @craigtaverner, I've updated the changelog YAML for you. |
| Hi @craigtaverner, I've updated the changelog YAML for you. |
craigtaverner force-pushed the merged_geo_lines_simplifier branch from 033baa2 to 4b7333e Compare 
There was a problem hiding this comment.
Maybe use static comparators? And also naturalOrdering and reverseOrder comparators (they make the code more readable in my opinion).
salvatore-campagna approved these changes Jun 22, 2023
| LGTM, I just left a couple of comments about using static comparators... |
There was a problem hiding this comment.
It is not clear to me how do you test the case where you have the line split among two data nodes.
And test sort-order ASC
As well as shuffling the incoming geo_lines to verify that the merge sort re-orders them.
craigtaverner force-pushed the merged_geo_lines_simplifier branch from 4b7333e to d59fc5f Compare craigtaverner added auto-backport-and-merge auto-merge-without-approval 💚 Backport successful
|
craigtaverner added a commit to craigtaverner/elasticsearch that referenced this pull request Jun 22, 2023
…lastic#96953) Recently we added support for line-simplification in geo-lines when in time-series aggregations: elastic#94954. This work did not, however, cover the case of `MergedGeoLines`, which occurs when the same geo_line covers multiple shards on data nodes, and needs to be merged on the coordinating node. The original work would use line-simplification and time-ordering on the data node, but then revert to re-sorting (unnecessary) and truncation (incorrect) on the coordinating node. This PR rectifies that, and adds two fields to the InternalGeoLine: * nonOverlapping * re-sorting is *NOT* required because the sort value ranges do not overlap, so we only sort the set of incoming geo_lines (easy/fast) instead of every single point (complex/slow). * simplified * The approach to memory limiting is by line-simplification instead of truncation. * If the data nodes are doing line-simplification, we want the coordinating node to do that as well For time-series, both of the above are true, and at this point all other cases will have both false. The reason for two booleans is: * They are not necessarily related concepts * We consider supporting line-simplification as a user-requested option in the non-time-series geo_line in future (much nicer than truncation) Fixes elastic#96983
elasticsearchmachine pushed a commit that referenced this pull request Jun 22, 2023
…96953) (#97005) Recently we added support for line-simplification in geo-lines when in time-series aggregations: #94954. This work did not, however, cover the case of `MergedGeoLines`, which occurs when the same geo_line covers multiple shards on data nodes, and needs to be merged on the coordinating node. The original work would use line-simplification and time-ordering on the data node, but then revert to re-sorting (unnecessary) and truncation (incorrect) on the coordinating node. This PR rectifies that, and adds two fields to the InternalGeoLine: * nonOverlapping * re-sorting is *NOT* required because the sort value ranges do not overlap, so we only sort the set of incoming geo_lines (easy/fast) instead of every single point (complex/slow). * simplified * The approach to memory limiting is by line-simplification instead of truncation. * If the data nodes are doing line-simplification, we want the coordinating node to do that as well For time-series, both of the above are true, and at this point all other cases will have both false. The reason for two booleans is: * They are not necessarily related concepts * We consider supporting line-simplification as a user-requested option in the non-time-series geo_line in future (much nicer than truncation) Fixes #96983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Recently we added support for line-simplification in geo-lines when in time-series aggregations: #94954. This work did not, however, cover the case of
MergedGeoLines, which occurs when the same geo_line covers multiple shards on data nodes, and needs to be merged on the coordinating node. The original work would use line-simplification and time-ordering on the data node, but then revert to re-sorting (unnecessary) and truncation (incorrect) on the coordinating node.This PR rectifies that, and adds two fields to the InternalGeoLine:
For time-series, both of the above are true, and at this point all other cases will have both false. The reason for two booleans is:
Fixes #96983