Bumps jts version to 1.20.0 #138351

ncordon · 2025-11-20T10:09:27Z

We need this to merge #136309.

Why

Our wkt representation should be locale independant.

Newer versions of jts correct at least an important bug when transforming jts's Geometry to wkt (we would have discrepancies in dots vs commas for floating part otherwise): locationtech/jts#473.

Also I've seen they solve an issue where the minus character for negative numbers was also locale dependant.

Our jts version was 8 years outdated anyway, so it's good to update it.

elasticsearchmachine · 2025-11-20T10:10:23Z

Hi @ncordon, I've created a changelog YAML for you.

elasticsearchmachine · 2025-11-20T10:17:32Z

Hi @ncordon, I've created a changelog YAML for you.

elasticsearchmachine · 2025-11-21T10:37:59Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

ncordon · 2025-11-21T10:43:47Z

modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/builders/PolygonBuilderTests.java

        InvalidShapeException e = expectThrows(InvalidShapeException.class, pb::buildS4J);
-        assertEquals("Self-intersection at or near point (-1.0, -1.0, NaN)", e.getMessage());
+        assertEquals("Self-intersection at or near point (0.0, 0.0, NaN)", e.getMessage());
    }


I think this error is still correct?

Yes, I think this is still correct, since either of the two points could be determined as the self-intersection point.

Since legacy-geo has been deprecated for a long time (6.x, I think?) a subtle change like this should be a non-issue. I think a change like this should not even matter for our main spatial module, since it does not affect any happy paths, only the possible error message for failing paths.

quackaplop

Sorry for the drive-by reviewing – I just accidentally came across this one.
Two silly questions

How convinced are we that the behavior between 1.15 and 1.20 is not going to break much on our end? I am not super familiar with JTS, so this is mainly paranoia.
You did have to change once test - I think this smells like a possible breaking change? is it?

craigtaverner

I think it is good to keep JTS up-to-date. It is nice that the only test change was in an error message, and in a case where either the old error or the new one would mean essentially the same thing.

I do think we should consider removing the old verification data.

craigtaverner · 2025-11-21T10:56:30Z

gradle/verification-metadata.xml

            <sha256 value="00102cde26c457b81fbb0248e4f8845884243caba0dc9b7fb42e0ea877383bc1" origin="Generated by Gradle"/>
         </artifact>
      </component>
+      <component group="org.locationtech.jts" name="jts-core" version="1.20.0">


Should we remove the verification data for the old JTS (line 4264 above)?

I still think this is a good idea, to remove the old line.

Yes, please remove the unused line.

craigtaverner · 2025-11-21T11:03:27Z

modules/legacy-geo/src/test/java/org/elasticsearch/legacygeo/builders/PolygonBuilderTests.java

        InvalidShapeException e = expectThrows(InvalidShapeException.class, pb::buildS4J);
-        assertEquals("Self-intersection at or near point (-1.0, -1.0, NaN)", e.getMessage());
+        assertEquals("Self-intersection at or near point (0.0, 0.0, NaN)", e.getMessage());
    }


Yes, I think this is still correct, since either of the two points could be determined as the self-intersection point.

Since legacy-geo has been deprecated for a long time (6.x, I think?) a subtle change like this should be a non-issue. I think a change like this should not even matter for our main spatial module, since it does not affect any happy paths, only the possible error message for failing paths.

iverase · 2025-11-21T13:24:48Z

We need to make sure this does not break the vector tiles API. I will run some test locally.

ncordon · 2025-11-25T10:49:09Z

How convinced are we that the behavior between 1.15 and 1.20 is not going to break much on our end? I am not super familiar with JTS, so this is mainly paranoia.

@iverase did find it introduced a bug in vector tiles that he had a fix for, I've added a test to avoid that regression happens again.

The alternative would be to stick to this version and patch the parts we need on our end (for example the WKT format seemed locale dependant in the old version for commas and dots, it has been fixed in the new ones). Given the library was highly outdated (8 years between 1.15 and 1.20) we lean heavily towards going ahead and updating it.

You did have to change once test - I think this smells like a possible breaking change? is it?

It was not breaking since the error is still correct and @craigtaverner confirmed that.

ncordon · 2025-11-25T12:38:39Z

...ector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java

+        var coordinates = mvtGeometry.getCoordinates();
+        var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
+        assertFalse(containsCenter);


We could assert here on the exact contents of the vector tiles, but they are jts version dependent, and it changes from 1.15 so 1.20, so I tried to add a version independent check instead.

craigtaverner

Great fix. I did have suggestions for a few comments and assertion messages to help make things clearer to any future developers stumbling across this code and wondering about it. The fix is quite subtle, so I think the comments are useful.

craigtaverner · 2025-11-25T12:56:15Z

gradle/verification-metadata.xml

            <sha256 value="00102cde26c457b81fbb0248e4f8845884243caba0dc9b7fb42e0ea877383bc1" origin="Generated by Gradle"/>
         </artifact>
      </component>
+      <component group="org.locationtech.jts" name="jts-core" version="1.20.0">


I still think this is a good idea, to remove the old line.

craigtaverner · 2025-11-25T12:56:55Z

...gin/vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactory.java

    }

+    /**
+     * Testing purposes only


Could elaborate a little

Suggested change

* Testing purposes only

* Testing purposes only, allowing us to test the impact of the geometry clipping and simplification

without needing to parse the protobuffer results.

craigtaverner · 2025-11-25T12:59:10Z

...gin/vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactory.java

+     * Testing purposes only
+     */
+    org.locationtech.jts.geom.Geometry getMvtGeometry(Geometry geometry) {
+        // Get geometry in pixel coordinates


Suggested change

// Get geometry in pixel coordinates

// Get clipped and simplified geometry in pixel coordinates

craigtaverner · 2025-11-25T13:01:46Z

...gin/vector-tile/src/main/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactory.java

        public void filter(CoordinateSequence seq, int i) {
-            seq.setOrdinate(i, 0, lon(seq.getOrdinate(i, 0)));
-            seq.setOrdinate(i, 1, lat(seq.getOrdinate(i, 1)));
+            if (i != 0 && i == seq.size() - 1 && seq.getCoordinate(0) == seq.getCoordinate(i)) {


I think a comment here might be useful. It is especially not clear why we need to do reference equality vs contents equality. We could add a comment like:

// JTS can sometimes close polygons using references to the starting coordinate, which can lead to that coordinate being converted twice. // We need to detect if the end is the same actual Coordinate, and not do the conversion again.

craigtaverner · 2025-11-25T13:04:41Z

...ector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java

+        var mvtGeometry = builder.getMvtGeometry(polygon);
+        var coordinates = mvtGeometry.getCoordinates();
+        var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
+        assertFalse(containsCenter);


Suggested change

assertFalse(containsCenter);

assertFalse(containsCenter, "Coordinate (2024,2024) only occurs with the double conversion bug");

craigtaverner · 2025-11-25T13:05:08Z

...ector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java

+        var mvtGeometry = builder.getMvtGeometry(geometry);
+        var coordinates = mvtGeometry.getCoordinates();
+        var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
+        assertFalse(containsCenter);


Suggested change

assertFalse(containsCenter);

assertFalse(containsCenter, "Coordinate (2024,2024) only occurs with the double conversion bug");

craigtaverner · 2025-11-25T13:05:22Z

...ector-tile/src/test/java/org/elasticsearch/xpack/vectortile/feature/FeatureFactoryTests.java

+        var mvtGeometry = builder.getMvtGeometry(polygon);
+        var coordinates = mvtGeometry.getCoordinates();
+        var containsCenter = Arrays.stream(coordinates).anyMatch(c -> c.x == 2048 && c.y == 2048);
+        assertFalse(containsCenter);


Suggested change

assertFalse(containsCenter);

assertFalse(containsCenter, "Coordinate (2024,2024) only occurs with the double conversion bug");

iverase

LGTM

iverase · 2025-11-25T13:14:02Z

gradle/verification-metadata.xml

            <sha256 value="00102cde26c457b81fbb0248e4f8845884243caba0dc9b7fb42e0ea877383bc1" origin="Generated by Gradle"/>
         </artifact>
      </component>
+      <component group="org.locationtech.jts" name="jts-core" version="1.20.0">


Yes, please remove the unused line.

elasticsearchmachine added the v9.3.0 label Nov 20, 2025

ncordon added :Analytics/Geo Indexing, search aggregations of geo points and shapes >upgrade Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Nov 20, 2025

Updates jts version to 1.20.0

44b0ba4

ncordon force-pushed the update-jts branch from 7d2eb27 to 44b0ba4 Compare November 20, 2025 10:16

ncordon changed the title ~~Bumps jts version~~ Bumps jts version to 1.20.0 Nov 20, 2025

ncordon and others added 2 commits November 20, 2025 11:17

Update docs/changelog/138351.yaml

40c0f3e

Small tweak to one of the tests

6c6c61a

ncordon force-pushed the update-jts branch from 92b069b to 6c6c61a Compare November 21, 2025 09:05

Merge branch 'main' into update-jts

00ec3f6

ncordon marked this pull request as ready for review November 21, 2025 10:37

ncordon requested a review from a team as a code owner November 21, 2025 10:37

ncordon requested review from craigtaverner and iverase November 21, 2025 10:37

ncordon commented Nov 21, 2025

View reviewed changes

quackaplop reviewed Nov 21, 2025

View reviewed changes

craigtaverner approved these changes Nov 21, 2025

View reviewed changes

ncordon added 2 commits November 25, 2025 11:41

Temp

5e8a4f9

Adds tests to avoid bugs due to using the same reference

509cf29

Merge branch 'main' into update-jts

e188b77

ncordon commented Nov 25, 2025

View reviewed changes

craigtaverner approved these changes Nov 25, 2025

View reviewed changes

iverase approved these changes Nov 25, 2025

View reviewed changes

Addresses pr review comments

e66f065

ncordon merged commit e6316ad into elastic:main Nov 25, 2025
34 checks passed

ncordon added a commit to ncordon/elasticsearch that referenced this pull request Nov 26, 2025

Bumps jts version to 1.20.0 (elastic#138351)

cc750e3

ncordon added a commit to ncordon/elasticsearch that referenced this pull request Nov 26, 2025

Bumps jts version to 1.20.0 (elastic#138351)

9a1a1e6

ncordon added a commit to ncordon/elasticsearch that referenced this pull request Nov 26, 2025

Bumps jts version to 1.20.0 (elastic#138351)

a1e9d29

	* Testing purposes only
	* Testing purposes only, allowing us to test the impact of the geometry clipping and simplification
	without needing to parse the protobuffer results.

	// Get geometry in pixel coordinates
	// Get clipped and simplified geometry in pixel coordinates

	assertFalse(containsCenter);
	assertFalse(containsCenter, "Coordinate (2024,2024) only occurs with the double conversion bug");

Bumps jts version to 1.20.0 #138351

Bumps jts version to 1.20.0 #138351

Uh oh!

Conversation

ncordon commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Uh oh!

elasticsearchmachine commented Nov 20, 2025

Uh oh!

elasticsearchmachine commented Nov 20, 2025

Uh oh!

elasticsearchmachine commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quackaplop left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase commented Nov 21, 2025

Uh oh!

ncordon commented Nov 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iverase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ncordon commented Nov 20, 2025 •

edited

Loading

quackaplop left a comment •

edited

Loading