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

Remove legacy geo code from AggregationResultUtils #77702

Merged
merged 6 commits into from
Sep 16, 2021

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Sep 14, 2021

AggregationResultUtils use some code for serialising the results of some geo aggregations that is considered legacy and it will be removed in the future. We should move this code to use the elasticsearch geo library.

This approach uses the simpler way to move to the new libraries but unfortunately it changes the format of the serialisation slightly which I guess might be problematic for backwards compatibility.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Rectangle r = GeoTileUtils.toBoundingBox(key.toString());
Polygon polygon = new Polygon(
new LinearRing(
new double[]{r.getMinLon(), r.getMaxLon(), r.getMaxLon(), r.getMinLon(), r.getMinLon()},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the order matter here? It seems that this 2-D array is not a transposition of the 2-D array on the left-hand side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably it does no matter because they represent the same polygon.

@iverase
Copy link
Contributor Author

iverase commented Sep 15, 2021

I am not happy changing the output of the aggregation, maybe we should just replace all the *..getPreferredName() with local static strings?

@hendrikmuhs
Copy link

I am missing context for this change. It seems to me that the upstream format for encoding geo shapes is changing. The docs don't mention a change (yet?).

E.g.

"linestring" -> "Linestring"

Does that mean we get rid of the "Elasticsearch Type" and make it the "GeoJSON Type"?
What's the timeline for this? Deprecated in 7, removed in 8?

Do you have a full example to see the differences?

As for "changing the output": Transform takes the output of the aggregation and produces index requests out of it. That means this basically only changes _source in the index request. For querying the transformed index this makes no difference, because the parsed results and index structures remain the same if I get this right. So when it comes to concerns regarding BWC: It's only a problem if a user reads and parses _source. I wonder if there is anyone doing that. Nevertheless it is a breaking change, my suggestion is therefore to do this in 8.x and document it.

@iverase
Copy link
Contributor Author

iverase commented Sep 15, 2021

Does that mean we get rid of the "Elasticsearch Type" and make it the "GeoJSON Type"?

No really, our json parser is (maybe too) flexible and accepts both signatures, lowercase and camelcase. Therefore both types are supported. What are we moving to legacy is everything under org.elasticsearch.common.geo.builders.* (and I guess we should have deprecated it) in favour of the elasticsearch geo library. The issue is that those builders depend on JTS and we want to remove JTS from server.

I think it is not necessary to make a breaking change but stop using the builders to get field names and just have them locally?

@hendrikmuhs
Copy link

ok, got it, so the change is backwards compatible.

I am not happy changing the output of the aggregation, maybe we should just replace all the *..getPreferredName() with local static strings?

I understand now what you mean. Sounds good to me.

@iverase
Copy link
Contributor Author

iverase commented Sep 15, 2021

Done, added some static strings to replace the legacy objects.

@iverase
Copy link
Contributor Author

iverase commented Sep 15, 2021

@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample

@iverase
Copy link
Contributor Author

iverase commented Sep 15, 2021

@elasticmachine update branch

Copy link

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

@iverase iverase merged commit d8229ac into elastic:master Sep 16, 2021
@iverase iverase deleted the AggregationResultUtils branch September 16, 2021 09:06
iverase added a commit to iverase/elasticsearch that referenced this pull request Sep 16, 2021
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
7.x

wjp719 added a commit to wjp719/elasticsearch that referenced this pull request Sep 18, 2021
* master: (185 commits)
  Implement get and containsKey in terms of the wrapped innerMap (elastic#77965)
  Adjust Lucene version and enable BWC tests (elastic#77933)
  Disable BWC to upgrade to Lucene-8.10-snapshot
  Reenable MlDistributedFailureIT
  [DOCS] Fix typo for `script.painless.regex.enabled` setting value (elastic#77853)
  Upgrade to Lucene-8.10.0-snapshot-bf2fcb53079 (elastic#77801)
  [DOCS] Fix ESS install lead-in (elastic#77887)
  Resolve thirdparty gradle plugin artifacts from mavencentral (elastic#77865)
  Reduce the number of times that `LifecycleExecutionState` is parsed when running a policy. (elastic#77863)
  Utility methods to add and remove backing indices from data streams (elastic#77778)
  Use Objects.equals() instead of == to compare strings (elastic#77840)
  [ML] prefer least allocated model when a new node is added to the cluster (elastic#77756)
  Deprecate ignore_throttled parameter (elastic#77479)
  Improve LifecycleExecutionState parsing. (elastic#77855)
  [DOCS] Removes deprecated word from HLRC title. (elastic#77851)
  Remove legacy geo code from AggregationResultUtils (elastic#77702)
  Adjust SearchableSnapshotsBlobStoreCacheIntegTests.testBlobStoreCache (elastic#77758)
  Laxify SecureSM to allow creation of the JDK's innocuous threads (elastic#77789)
  [Test] Reduce concurrency when testing creation of security index (elastic#75293)
  Refactor metric PipelineAggregation integration test (elastic#77548)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants