Asset tracking: line-simplification for geo_line in TSDB #94954
Merged
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 
| Hi @craigtaverner, I've created a changelog YAML for you. |
craigtaverner force-pushed the geo_line_tsdb branch 2 times, most recently from 5dc6128 to d5fb222 Compare craigtaverner force-pushed the geo_line_tsdb branch from 03b2dfd to 48a8817 Compare | Pinging @elastic/es-analytics-geo (Team:Analytics) |
craigtaverner changed the title 
iverase reviewed May 31, 2023
test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java Outdated Show resolved Hide resolved
iverase reviewed May 31, 2023
iverase reviewed May 31, 2023
...ial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregator.java Outdated Show resolved Hide resolved
iverase reviewed May 31, 2023
.../main/java/org/elasticsearch/xpack/spatial/search/aggregations/TimeSeriesGeoLineBuckets.java Outdated Show resolved Hide resolved
iverase reviewed May 31, 2023
.../main/java/org/elasticsearch/xpack/spatial/search/aggregations/GeoLineAggregatorFactory.java Outdated Show resolved Hide resolved
iverase requested changes May 31, 2023
| Thanks for the review. I'll certainly add the missing sort-field checks and associated tests, and make other fixes. Probably only next week though. |
craigtaverner force-pushed the geo_line_tsdb branch from 8783d21 to e1dd4d4 Compare 
Starting with YAML tests (which currently pass) and AggregatorTests (currently failing, likely due to mistake in the tests)
* Created TimeSeries version of GeoLineAggregator, and wired it in so that time-series aggregations use it, but current behavior is still identical to non-time-series. * Added both yaml and unit tests for testing that geo_line works with correct results in both time-series and non-time-series cases. * Added additional tests to verify the grouping behaviour of time-series vs. terms aggs, and the combination of the two.
* Started refactoring to re-use simplifier for all buckets
The bucket id can change within a segment, so we need to detect this and save the geo_line.
The original geo_line relied on the BucketedSort for all intelligence. The time-series geo_line uses none of that, and does its own memory management.
And enhanced unit tests to cover multiple groups
Only activate the time-series optimizations if the aggregation is both: * Within a time-series aggregation (ie. tsid and @timestamp ordered) * The geo_line sort field is @timestamp
craigtaverner force-pushed the geo_line_tsdb branch from 6b16df7 to 014c395 Compare Also disables the new geo_line for time-series even if the correct sort and point fields are used if the point field is not explicitly configured to be a position metric.
iverase reviewed Jun 14, 2023
| defaultValueSourceType() | ||
| ); | ||
| configs.put(SORT_FIELD.getPreferredName(), sourceConfig); | ||
| } else if (false && sourceConfig.fieldContext().field().equals(TIMESTAMP_FIELD.getName()) == false) { |
There was a problem hiding this comment.
I think we should add test on the builder for the different combinations, for example if the sort field is null and not a time series I expect it to fail.
There was a problem hiding this comment.
Since the geo_line builder only changes behaviour when nested inside the time_series builder, the builder tests cannot cover this (they test only parsing, which has not changed, and already allow missing sort fields, so no change there).
However, we added negative tests in the yaml (assert exception is thrown on incompatible sort field), and positive tests in both yaml and aggregator tests (the aggregator tests also verify that different geo_line are produced from the same data when passed through the different algorithms, and that the same geo_line is produced if no truncation/simplification happens, even with different algorithms).
Curiously I think there never was a test for a null sort-field throwing assertion for classic geo_line. That is missing test coverage from the old code. I could add that in this PR, or make a new one for that. In the case of time-series I think I have good coverage, so this PR is OK from that perspective.
There was a problem hiding this comment.
Let's do that as a follow up, this PR is already pretty big.
Since the primary criteria for switching to the new algorithm is that geo_line is within a time-series aggregation, we now disallow any other sort field. We test the negative case in the yaml tests, but changed the unit tests to use TermsAggregation to minim the time-series aggregation to get comparable results.
The old code only threw error if there was data because the check was done inside the leaf collector just before actually reading the sort field. And there were no tests for missing sort field. This commit adds the tests, and checks early so even if data is missing.
* Test that behaviour is identical with or without POSITION metric * Removed fallback code in builder (was switching to old geo_line without POSITION metric) * Removed two TODO's that are no longer valid concerns
This was referenced Jun 15, 2023
craigtaverner changed the title elasticsearchmachine pushed a commit that referenced this pull request Jun 22, 2023
…96953) 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
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
| @craigtaverner according to this PR's labels, I need to update the changelog YAML, but I can't because the PR is closed. Please either update the changelog yourself on the appropriate branch, or adjust the labels. Specifically:
|
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.
Part of the work described in #85274
Within time-series data, two optimisations are done:
Tasks to complete before merging:
Final decision on when to activate optimized geo_line:
geo_line on position metric, decided to not require this, and instead:@timestampSo now the switch between old and new geo_line behaviour is exclusively based on its nesting inside a time_series aggregation.
Work to be done in additional PRs:
Use registered(cancelled based on decision above)ValueSource->Aggregatormappings to map position metrics toGeoLineAggregator.TimeSeriesMergedGeoLinesfor time-series data (ie. append and re-simplify) - Fix time-series geo_line to include reduce phase in MergedGeoLines #96953