Provide a pure Java Elasticsearch spatial plugin#2388
Provide a pure Java Elasticsearch spatial plugin#2388jpolchlo wants to merge 17 commits intonasa:masterfrom
Conversation
…atibility MILESTONE: Implement ords-info->shapes in pure Java spatial-lib now includes a pure Java implementation of shape deserialization to support ES8 plugin requirements. The implementation maintains full backward compatibility with existing Clojure code by converting Java shapes back to Clojure records. New Java Components: - cmr.spatial.serialize.OrdsInfoShapes: Main deserialization logic - cmr.spatial.java.SpatialShape: Base interface for all shape types - cmr.spatial.java.Point, Mbr, Polygon, Ring, LineString: Shape POJOs Changes to spatial-lib: - project.clj: Added :java-source-paths ["src/java"] - src/cmr/spatial/serialize.clj: - ords-info->shapes now delegates to Java implementation - Added java-shape->clojure-shape converter for backward compatibility - Existing callers receive the same Clojure shape records as before Backward Compatibility: - search-app, umm-lib, umm-spec-lib work unchanged - API is identical: returns the same Clojure records - All serialize tests pass For ES8 Plugin: - es-spatial-plugin can call OrdsInfoShapes directly without Clojure runtime - Eliminates dependency on Clojure runtime for shape deserialization - Unblocks pure Java implementation of ES plugin TODO - Future Work: - Implement shape->intersects-fn in Java for ES8 plugin - Currently shape->intersects-fn delegates to Clojure (see ShapeIntersections.java) - Next PR will add intersection testing in pure Java
Added intersectsLineString and coversLineString methods to handle ring-linestring operations without delegating to Clojure.
All polygon intersection methods now use Java ring implementations via createJavaRing factory. Removes Clojure delegation from polygons.
Implements circle as center point + radius. Uses Haversine distance for point coverage and converts to polygon approximation for other shape intersections.
Added all missing shape-to-shape combinations (Point, Mbr, LineString, Ring, Polygon, Circle). ES plugin now has full Java coverage.
Java and Clojure APIs are now separate. Replaced delegateToClojure calls with UnsupportedOperationException for clarity.
Moved java/ -> shape/ for public API shapes Created internal/ parent for implementation details: - jarc/ -> internal/arc/ (geodetic primitives) - jsegment/ -> internal/segment/ (cartesian primitives) - ring/ -> internal/ring/ (complex implementations) Removed J prefix from GeodeticRing and CartesianRing. Fixed protocol extensions to support both Java and Clojure Arc types.
Added getMbr1/getMbr2 getters to Arc.java. Updated Clojure code to use keyword access for Clojure Arc fields instead of Java methods.
High priority fixes: - Point: Fix hashCode/equals contract with normalized values - LineStringIntersections: Compute proper geodetic MBRs using arcs - RingIntersections: Use precise arc.pointOnArc() instead of MBR check - PointIntersections: Use tolerance for antimeridian detection Medium priority fixes: - CircleIntersections: Validate radius and numPoints parameters - LineSegment: Guard against divide-by-zero in densifyLineSegment - MathUtils: Clamp angularDistance to prevent NaN - ShapeIntersections: Add null check for Circle intersections - RingIntersections: Skip invalid arcs in getLineStringSegments - ArcLineSegmentIntersections: Handle degenerate vertical segments - OrdsInfoShapes: Validate ordsInfo is even length, use mutable lists - ESPluginJavaIntersectionsTest: Fix package import
- Ring: Add isHole field and constructor parameter - Ring.getType(): Return correct suffix based on isHole flag (-ring vs -hole) - LineString/Ring.getOrdinates(): Return unmodifiable views - Polygon.getRings(): Return unmodifiable view - Polygon: Add getMutableRings() for OrdsInfoShapes deserialization - OrdsInfoShapes: Use Ring constructor with isHole flag
Add :java-ring field to CartesianRing and GeodeticRing records. Calculate java-ring once in calculate-derived and reuse in covers-point?. Fallback to on-the-fly creation if ring not yet derived. This eliminates repeated Java ring reconstruction on hot paths, particularly in ring_relations intersection operations.
- LineStringIntersections: Skip degenerate arc creation, implement antimeridian-aware MBR union - RingIntersections: Use tolerance-based single-point detection, delegate to pole/antimeridian-aware point equality - ArcLineSegmentIntersections: Add null guards, continue through pole intersections to find additional points - OrdsInfoShapes: Add bounds checking and orphan-hole validation All spatial-lib tests pass.
When granules don't have spatial data, Elasticsearch returns nil for ords-info, which was causing IllegalArgumentException in the Java spatial library. The original Clojure implementation handled this gracefully via nil-punning where (partition 2 nil) returns empty seq. Changes: - Return empty ArrayList when ordsInfo is null instead of throwing - Add test case for nil ords-info handling - Update JavaDoc to clarify nil is valid for spatially-unlocated granules Fixes granule searches for providers like NCEI that have granules without spatial data. Tested with JSON, ATOM, KML, and STAC formats.
…ions Change computeMbrIntersection to use geodetic semantics (was cartesian). This fixes the case where Arc and LineSegment MBRs touch at a pole but have disjoint longitude ranges - in geodetic coordinates, MBRs touching the same pole are considered to intersect at that pole. The cartesian vs geodetic distinction matters for pole-touching logic, not antimeridian handling (Arc pre-splits crossing MBRs, LineSegment never crosses by design). Changes: - Use "geodetic" instead of "cartesian" for MBR intersection test - Add detailed comment explaining geodetic semantics and assumptions - Add test cases for pole-touching with disjoint longitude ranges Addresses code review feedback on coordinate system handling.
ES Plugin Changes: - Add :java-source-paths ["src/java"] to enable Java compilation - Add :javac-options ["-target" "11" "-source" "11"] for ES 8.18.7 compatibility - Update plugin-descriptor.properties java.version from 1.8 to 11 Without these changes, plugin JAR contained only .java source files, not compiled .class files, causing "Could not find plugin class" errors at runtime. Makefile Changes: - Update es-spatial-plugin target to track *.java files instead of *.clj - Use 'lein prepare-es-plugin' which includes gather-dependencies step - Fix docker build to run from correct directory context Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Deleted files: - src/cmr/elasticsearch/plugins/spatial/engine/core.clj - src/cmr/elasticsearch/plugins/spatial/factory/core.clj - src/cmr/elasticsearch/plugins/spatial/factory/lfactory.clj - src/cmr/elasticsearch/plugins/spatial/plugin.clj - src/cmr/elasticsearch/plugins/spatial/script/core.clj - resources/plugin/plugin-security.policy These Clojure gen-class files were replaced by SpatialSearchPlugin.java in commit 90383b649. The security policy file is not used in ES 8.x. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request migrates the Elasticsearch spatial plugin from a pure Clojure implementation to a hybrid Java-backed architecture, introduces comprehensive spatial geometry utilities in Java, and updates build configurations to support Java source compilation and containerization. Changes
Suggested labels
Suggested reviewers
Poem
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
spatial-lib/src/cmr/spatial/lr_binary_search.clj-267-273 (1)
267-273:⚠️ Potential issue | 🟡 MinorGuard fallback point derivation to avoid crashing the recovery path.
If
:ringsor:pointsis empty,first-pointis nil and Line 273 can throw inm/point->mbr, which breaks this fallback branch.🔧 Proposed hardening
;; Get first point - handle both polygon and ring (let [first-point (if (:rings polygon) ;; It's a polygon - get first point of first ring (-> polygon :rings first :points first) ;; It's a ring - get first point directly (-> polygon :points first))] - (m/point->mbr first-point)))))))) + (if first-point + (m/point->mbr first-point) + (do + (warn "Could not derive fallback point; returning shape MBR.") + mbr)))))))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/lr_binary_search.clj` around lines 267 - 273, The fallback branch can pass nil to m/point->mbr when polygon has empty :rings or :points; update the code around the local first-point calculation to guard against nil: after computing first-point (from (-> polygon :rings first :points first) or (-> polygon :points first)), check for nil and only call m/point->mbr when first-point is non-nil, otherwise return a safe fallback (e.g., nil or an empty MBR) or skip this recovery branch; modify the block that assigns first-point and the subsequent m/point->mbr call to perform this nil-check to avoid throwing in m/point->mbr.spatial-lib/src/cmr/spatial/ring_relations.clj-171-175 (1)
171-175:⚠️ Potential issue | 🟡 Minor
^Mbrtype hint documents only the Clojure MBR type.The hint
^Mbron line 173 resolves tocmr.spatial.mbr.Mbr(the imported Clojure defrecord), while->java-mbrtransparently acceptscmr.spatial.shape.Mbrtoo. This is a minor documentation inconsistency — no runtime impact since actual callers pass Clojure MBRs — but could mislead future contributors about valid argument types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/ring_relations.clj` around lines 171 - 175, The ^Mbr type hint on the intersects-br? arg documents only the Clojure MBR defrecord though ->java-mbr also accepts cmr.spatial.shape.Mbr; update the hint to reflect the broader accepted type (for example replace ^Mbr with ^cmr.spatial.shape.Mbr) or remove the hint entirely so the signature no longer misdocuments valid inputs; the change should be made on the intersects-br? function's parameter and keep the call to ->java-mbr as-is.spatial-lib/src/cmr/spatial/points_validation_helpers.clj-23-25 (1)
23-25:⚠️ Potential issue | 🟡 Minor
conjcalled with a single argument is a no-op.
(conj coll)in Clojure simply returnscollunchanged, so theconjwrapper around theforexpression has no effect. This appears to be a leftover from an earlier version that likely had a collection as the first argument (e.g.,(conj [] ...)or(into [] ...)). If the intent is to return a realized vector, use(into [] ...)or(vec ...).🐛 Proposed fix to remove the no-op `conj`
- (conj (for [^long idx (range (dec (count points)))] - (vector (nth modified-points idx) - (nth modified-points (inc idx))))))) + (for [idx (range (dec (count points)))] + (vector (nth modified-points idx) + (nth modified-points (inc idx))))))Or, if a realized sequence is desired:
- (conj (for [^long idx (range (dec (count points)))] - (vector (nth modified-points idx) - (nth modified-points (inc idx))))))) + (into [] (for [idx (range (dec (count points)))] + (vector (nth modified-points idx) + (nth modified-points (inc idx)))))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/points_validation_helpers.clj` around lines 23 - 25, The current code wraps a for comprehension with (conj ...) which is a no-op when called with a single argument; replace the conj wrapper around the for expression that builds adjacent point pairs (the expression iterating over ^long idx (range (dec (count points))) and using (nth modified-points idx) and (nth modified-points (inc idx))) with a realizer such as (into [] ...) or (vec ...) so the function returns a realized vector of pairs instead of an unchanged sequence.spatial-lib/src/java/cmr/spatial/geometry/PointIntersections.java-54-55 (1)
54-55:⚠️ Potential issue | 🟡 Minor
pointsApproxEqualignores itsdeltafor antimeridian matching.Line [54]-[55] routes through Line [89]-[90], which hardcodes
MathUtils.DELTAinstead of the method’sdeltaargument.Proposed fix
- if (isOnAntimeridian(lon1) && isOnAntimeridian(lon2)) { + if (isOnAntimeridian(lon1, delta) && isOnAntimeridian(lon2, delta)) { return true; } @@ - private static boolean isOnAntimeridian(double lon) { - return Math.abs(Math.abs(lon) - 180.0) < MathUtils.DELTA; + private static boolean isOnAntimeridian(double lon, double delta) { + return Math.abs(Math.abs(lon) - 180.0) < delta; }Also applies to: 89-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/PointIntersections.java` around lines 54 - 55, pointsApproxEqual currently bypasses the passed-in delta when both longitudes are on the antimeridian by calling isOnAntimeridian and then using MathUtils.DELTA; update the antimeridian comparison logic inside pointsApproxEqual (and the secondary antimeridian branch that routes to the same check) to use the method's delta parameter instead of MathUtils.DELTA so the supplied tolerance is honored for antimeridian equality checks; ensure both places referencing MathUtils.DELTA (including the code path invoked when isOnAntimeridian(lon1) && isOnAntimeridian(lon2) and the later fallback at the other antimeridian check) are changed to use delta.spatial-lib/test/cmr/spatial/test/serialize.clj-78-81 (1)
78-81:⚠️ Potential issue | 🟡 Minor
deftestdoes not support docstrings — the string on line 79 is a no-op expression.Unlike
defn, Clojure'sdeftestdoesn't accept a docstring. The string"Test that nil ords-info..."is evaluated and discarded at runtime. Move it to a comment if you want it as documentation.Proposed fix
(deftest ords-info->shapes-nil-test - "Test that nil ords-info (granules without spatial data) returns empty list" + ;; Test that nil ords-info (granules without spatial data) returns empty list (is (= [] (srl/ords-info->shapes nil nil))) (is (= [] (srl/ords-info->shapes nil []))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/test/cmr/spatial/test/serialize.clj` around lines 78 - 81, The test uses a string literal inside deftest which is a no-op (deftest doesn't accept docstrings); update the deftest named ords-info->shapes-nil-test by removing or converting the string "Test that nil ords-info (granules without spatial data) returns empty list" into a comment (or move it above as a separate comment) so the test body only contains the assertions calling srl/ords-info->shapes; ensure no stray string literals remain inside the deftest.spatial-lib/src/java/cmr/spatial/geometry/CircleIntersections.java-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorInconsistent Earth radius between
coversPointand polygon-based methods.
coversPoint(line 80) delegates toMathUtils.distancewhich usesEARTH_RADIUS_METERS = 6367435.0, whilecircleToPolygon(line 116) usesEARTH_RADIUS_APPROX = 6378137.0(WGS84 equatorial radius). This ~11 km difference means a point near the circle boundary could be judged "covered" bycoversPointbut fall outside the polygon approximation (or vice versa). If this matches the original Clojure behavior, it's a known inconsistency, but it may cause edge-case disagreements between the directcoversPointpath and the polygon-delegatedintersects*paths.Also applies to: 80-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/CircleIntersections.java` at line 39, The code uses two different Earth radii causing inconsistent behavior between coversPoint and polygon-based methods; update CircleIntersections so both coversPoint (which uses MathUtils.distance / EARTH_RADIUS_METERS) and circleToPolygon (which uses EARTH_RADIUS_APPROX) use the same radius source: either remove EARTH_RADIUS_APPROX and call MathUtils or a shared constant for the radius inside circleToPolygon, or change MathUtils to expose/accept the radius and have coversPoint and circleToPolygon reference that single radius (refer to coversPoint, circleToPolygon, MathUtils.distance, EARTH_RADIUS_APPROX and EARTH_RADIUS_METERS).spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java-113-129 (1)
113-129:⚠️ Potential issue | 🟡 MinorSilently skipping invalid arcs may produce a ring with no arcs for degenerate input.
pointsToArcssilently swallowsIllegalArgumentExceptionfromArc.createArc(antipodal points, etc.) and skips equal-point pairs. If all pairs are degenerate,arcsis empty, leading tocalculateRotationDirectionreturningNONEandcalculateMbrreturning a whole-world MBR (-180, 90, 180, -90). This could cause a degenerate ring to appear to contain everything.Consider logging a warning when arcs are skipped, or validating that the resulting arc list isn't empty for a non-trivial point list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java` around lines 113 - 129, The pointsToArcs method currently swallows IllegalArgumentException from Arc.createArc and skips equal-point pairs (via pointsEqual), which can produce an empty arcs list and lead calculateRotationDirection to return NONE and calculateMbr to produce a whole-world MBR; update pointsToArcs to (1) record/log a warning when an arc is skipped (either due to pointsEqual or an IllegalArgumentException from Arc.createArc) including the point indices or coordinates, and (2) after building arcs, validate that for any non-trivial input (points.size() > 1) the resulting arcs list is not empty — if it is, propagate a clear error (throw an IllegalArgumentException) or return an explicit failure so callers (calculateRotationDirection / calculateMbr) don’t treat the ring as global; reference pointsToArcs, Arc.createArc, pointsEqual, calculateRotationDirection, and calculateMbr when making these changes.spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java-744-750 (1)
744-750:⚠️ Potential issue | 🟡 Minor
ordinatesToPointswill throwIndexOutOfBoundsExceptionon odd-length input.If the ordinates list has an odd number of elements,
ordinates.get(i + 1)on the last iteration will throw. Consider adding a guard or at least documenting the precondition.Proposed fix
private static List<Point> ordinatesToPoints(List<Double> ordinates) { + if (ordinates.size() % 2 != 0) { + throw new IllegalArgumentException( + "Ordinates list must have even length, got: " + ordinates.size()); + } List<Point> points = new ArrayList<>(); for (int i = 0; i < ordinates.size(); i += 2) { points.add(new Point(ordinates.get(i), ordinates.get(i + 1)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java` around lines 744 - 750, The method ordinatesToPoints currently assumes an even-length ordinates list and will throw IndexOutOfBoundsException when the list length is odd; fix by validating the input size at the start of ordinatesToPoints (e.g., if (ordinates.size() % 2 != 0) throw new IllegalArgumentException("ordinates must contain an even number of values")) or by changing the loop to use a safe condition (for (int i = 0; i + 1 < ordinates.size(); i += 2) ...) and decide whether to ignore the trailing value or fail fast — update the code accordingly and include a clear message referencing ordinatesToPoints.spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java-465-474 (1)
465-474:⚠️ Potential issue | 🟡 MinorSwapped variable names
rightLonandleftLon.
rightLonis computed asmid(w, midLon)— which is the midpoint between west and center (i.e., the left quarter-point). ConverselyleftLonismid(midLon, e)— the right quarter-point. The names are swapped relative to their geographic positions.This doesn't affect correctness since all three are valid external points, but it's confusing for anyone reading the code.
Proposed fix
double midLon = mid(w, e); - double rightLon = mid(w, midLon); - double leftLon = mid(midLon, e); + double leftLon = mid(w, midLon); + double rightLon = mid(midLon, e); double midLat = mid(n, s); - result.add(roundPointForExternalPoints(new Point(leftLon, midLat))); + result.add(roundPointForExternalPoints(new Point(leftLon, midLat))); result.add(roundPointForExternalPoints(new Point(midLon, midLat))); result.add(roundPointForExternalPoints(new Point(rightLon, midLat)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java` around lines 465 - 474, The two longitude variables are named backwards: the assignment for rightLon and leftLon should match their geographic positions; change the assignments around so leftLon = mid(w, midLon) and rightLon = mid(midLon, e) (these variables are used to construct Points passed to roundPointForExternalPoints in GeodeticRing); ensure mid(w,e) remains midLon and mid(n,s) remains midLat, then run tests to confirm behavior unchanged.spatial-lib/src/test/java/cmr/spatial/geometry/IntersectionTestsTest.java-196-203 (1)
196-203:⚠️ Potential issue | 🟡 MinorAssertion relies on reference equality for
Mbr— fragile if implementation changes.Line 202 uses
assertEqualsto compare the originalMbrwith the returned one. This works today becausesplitAcrossAntimeridianadds the same object reference for non-crossing MBRs, butMbrdoes not overrideequals, so it falls back to reference comparison. This is fragile: if the implementation ever wraps or copies the MBR, the test will break.Compare field values instead:
Proposed fix
- assertEquals("Part should be the same as original", mbr_not_crosses, parts.get(0)); + assertEquals("West should match", -30.0, parts.get(0).getWest(), 0.0); + assertEquals("North should match", 50.0, parts.get(0).getNorth(), 0.0); + assertEquals("East should match", 30.0, parts.get(0).getEast(), 0.0); + assertEquals("South should match", -10.0, parts.get(0).getSouth(), 0.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/test/java/cmr/spatial/geometry/IntersectionTestsTest.java` around lines 196 - 203, The test testSplitAcrossAntimeridian_NoCross currently compares Mbr objects by reference; update it to assert on the Mbr coordinate values instead of object equality: call MbrIntersections.splitAcrossAntimeridian(mbr_not_crosses), then extract parts.get(0) and assert each coordinate/field (e.g., getMinX/getMinY/getMaxX/getMaxY or the appropriate accessors on Mbr) equals the corresponding value from mbr_not_crosses using assertEquals with a small delta for doubles; keep the size assertion but replace the object-equality assertion with these per-field assertions to avoid relying on reference identity.spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java-554-580 (1)
554-580:⚠️ Potential issue | 🟡 MinorAdd tolerance to latitude check in
geodeticMbrCoversPointto match Clojure and other Java implementations.Line 559 checks
lat < mbr.getSouth() || lat > mbr.getNorth()with no tolerance, while line 564 appliesMBR_COVERS_TOLERANCEto longitude bounds. The Clojure implementation (cmr.spatial.mbr/geodetic-covers-point?) applies tolerance to latitude viacovers-lat?(mbr.clj line 159), and all other Java implementations—RingIntersections.coversLat(line 630),ArcLineSegmentIntersections.coversLat(line 665), andArc.coversLatMbr(line 585)—apply tolerance to latitude bounds. A point at the exact MBR boundary latitude could be incorrectly rejected, causingcoversPointto returnfalsewithout ray-casting.Fix
private static boolean geodeticMbrCoversPoint(Mbr mbr, Point point) { double lon = point.getLon(); double lat = point.getLat(); // Check latitude bounds - if (lat < mbr.getSouth() || lat > mbr.getNorth()) { + if (lat < mbr.getSouth() - MBR_COVERS_TOLERANCE || lat > mbr.getNorth() + MBR_COVERS_TOLERANCE) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java` around lines 554 - 580, The latitude check in geodeticMbrCoversPoint currently uses strict bounds (lat < mbr.getSouth() || lat > mbr.getNorth()) without applying MBR_COVERS_TOLERANCE; change it to apply the same tolerance used for longitude by computing a south/tolerance-adjusted south and north (e.g., south = mbr.getSouth() - MBR_COVERS_TOLERANCE, north = mbr.getNorth() + MBR_COVERS_TOLERANCE) and use those adjusted bounds when evaluating lat, leaving the rest of the function (west/east, crossesAntimeridian, antimeridian special case) unchanged so behavior matches covers-lat? and other Java implementations (see geodeticMbrCoversPoint, MBR_COVERS_TOLERANCE).
🧹 Nitpick comments (27)
spatial-lib/src/cmr/spatial/ring_relations.clj (2)
12-18: Add missing imports for all referenced Java classes.
cmr.spatial.internal.ring.GeodeticRing,cmr.spatial.internal.ring.CartesianRing, andcmr.spatial.shape.Mbrare all referenced by fully-qualified names in the new functions but are not declared in the:importblock. While Clojure permits FQN usage without explicit import (provided the classes are on the classpath), this is inconsistent with the import style the file already establishes, and the#_{:clj-kondo/ignore [:unused-import]}hint won't suppress missing import warnings from tools that check for them.♻️ Proposed fix
(:import cmr.spatial.cartesian_ring.CartesianRing cmr.spatial.geodetic_ring.GeodeticRing cmr.spatial.point.Point cmr.spatial.mbr.Mbr - [cmr.spatial.geometry RingIntersections])) + [cmr.spatial.geometry RingIntersections] + cmr.spatial.internal.ring.GeodeticRing + cmr.spatial.internal.ring.CartesianRing + cmr.spatial.shape.Mbr))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/ring_relations.clj` around lines 12 - 18, Add the missing Java class imports to the existing :import block so the file consistently declares the referenced classes: include cmr.spatial.internal.ring.GeodeticRing, cmr.spatial.internal.ring.CartesianRing, and cmr.spatial.shape.Mbr alongside the existing imports (e.g., RingIntersections, Point), ensuring the fully-qualified classes used in the new functions are explicitly imported.
95-96: Consider removing the Java-ring passthrough branches.These branches guard against
cmr.spatial.internal.ring.GeodeticRing/CartesianRingbeing passed directly tointersects-ring?orintersects-br?. No current Clojure call-site does this, so the code is YAGNI. If the intent is to support future mixed Clojure/Java callers, a comment explaining that would help; otherwise these branches add dead weight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/ring_relations.clj` around lines 95 - 96, The two Java-ring passthrough branches in intersects-ring? / intersects-br? (the instance? checks for cmr.spatial.internal.ring.GeodeticRing and cmr.spatial.internal.ring.CartesianRing) are dead-weight and should be removed: delete those two instance? branches from ring_relations.clj so the functions rely on the existing Clojure ring handling paths; if you intend to support Java callers in future, instead of keeping them silently, add a short comment near the top of intersects-ring? / intersects-br? explaining that Java GeodeticRing/CartesianRing are intentionally unsupported or will be handled elsewhere, then run the test-suite to ensure no regressions.spatial-lib/src/cmr/spatial/points_validation_helpers.clj (1)
23-25: Replaceforwithloop/recurfor primitive performance.The
^longtype hint on theforbinding doesn't produce a primitive local—forcomprehensions iterate over boxed values, and(range ...)returns a lazy sequence of boxedLongobjects. Sinceprimitive-math'sincunboxes its arguments when given boxed types, each iteration incurs boxing overhead. Useloop/recurto maintain primitivelonglocals throughout:(loop [^long idx 0] (if (< idx (dec (count points))) (conj result (vector (nth modified-points idx) (nth modified-points (inc idx)))) result))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/points_validation_helpers.clj` around lines 23 - 25, The current use of for with a ^long hint (iterating over (range (dec (count points)))) boxes the index and hurts primitive performance; replace that comprehension with a loop/recur that binds a primitive ^long idx and iterates until (< idx (dec (count points))), conj-ing each vector of (nth modified-points idx) and (nth modified-points (inc idx)) into the accumulating result, and return the accumulated result when done; update the code around symbols points and modified-points (and the surrounding function that builds the pair vectors) so the loop uses primitive long locals and avoids boxing on each iteration.spatial-lib/src/cmr/spatial/mbr.clj (1)
309-320: Extract Clojure-record → Java-MBR conversion into a helper.The same field mapping is duplicated in both functions; centralizing it lowers maintenance risk.
♻️ Proposed refactor
+(defn- ->java-mbr + [^cmr.spatial.mbr.Mbr m] + (cmr.spatial.shape.Mbr. (.west m) (.north m) (.east m) (.south m))) + (defn non-crossing-intersects-br? "Specialized version of intersects-br? for two mbrs that don't cross the antimeridian. Returns true if the mbr intersects the other bounding rectangle." [coord-sys ^cmr.spatial.mbr.Mbr m1 ^cmr.spatial.mbr.Mbr m2] (pj/assert (not (or (crosses-antimeridian? m1) (crosses-antimeridian? m2)))) ;; Delegate to Java implementation - (let [java-mbr1 (cmr.spatial.shape.Mbr. (.west m1) (.north m1) (.east m1) (.south m1)) - java-mbr2 (cmr.spatial.shape.Mbr. (.west m2) (.north m2) (.east m2) (.south m2))] + (let [java-mbr1 (->java-mbr m1) + java-mbr2 (->java-mbr m2)] (MbrIntersections/nonCrossingIntersects (name coord-sys) java-mbr1 java-mbr2))) (defn intersects-br? "Returns true if the mbr intersects the other bounding rectangle" [coord-sys ^cmr.spatial.mbr.Mbr mbr ^cmr.spatial.mbr.Mbr other-br] ;; Delegate to Java implementation which handles all edge cases - (let [java-mbr1 (cmr.spatial.shape.Mbr. (.west mbr) (.north mbr) (.east mbr) (.south mbr)) - java-mbr2 (cmr.spatial.shape.Mbr. (.west other-br) (.north other-br) - (.east other-br) (.south other-br))] + (let [java-mbr1 (->java-mbr mbr) + java-mbr2 (->java-mbr other-br)] (MbrIntersections/mbrsIntersect (name coord-sys) java-mbr1 java-mbr2)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/mbr.clj` around lines 309 - 320, Extract the duplicated Clojure-record→Java Mbr conversion into a single helper (e.g., a private function like make-java-mbr or mbr->java-mbr) and use it from both nonCrossingIntersects and intersects-br?; the helper should accept a Clojure cmr.spatial.mbr.Mbr (or two args if preferred) and return a new cmr.spatial.shape.Mbr constructed from (.west ...), (.north ...), (.east ...), (.south ...), then replace the inline constructions in nonCrossingIntersects and intersects-br? to call the helper for java-mbr1 and java-mbr2 before passing them to MbrIntersections/nonCrossingIntersects and MbrIntersections/mbrsIntersect respectively.es-spatial-plugin/Dockerfile (1)
6-6: Avoid full OS upgrade during image build.
apt upgrade -yintroduces non-deterministic image drift. Prefer relying on the pinned upstream Elasticsearch base image patch cycle.♻️ Proposed simplification
USER root -RUN apt update && apt upgrade -y && apt clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@es-spatial-plugin/Dockerfile` at line 6, The Dockerfile currently runs "RUN apt update && apt upgrade -y && apt clean", which causes non-deterministic OS upgrades; remove the "apt upgrade -y" and instead only update the package index and install explicit, required packages (using apt-get install -y --no-install-recommends for any needed packages) and then clean apt lists (e.g., remove /var/lib/apt/lists/*) to keep the image deterministic and rely on the upstream Elasticsearch base image for OS patching; update the RUN line that contains "apt update && apt upgrade -y && apt clean" accordingly.spatial-lib/src/java/cmr/spatial/internal/segment/LineSegment.java (3)
404-411: Duplicate Javadoc comment block.There are two consecutive Javadoc comments for
intersectionOneVertical— the first (lines 404-406) is an incomplete/stale duplicate of the second (lines 407-410).Remove duplicate Javadoc
- /** - * Returns the intersection point of one vertical and one non-vertical line. - */ /** * Returns the intersection of one vertical line and another non-vertical line. * Matches Clojure intersection-one-vertical. */ private static Point intersectionOneVertical(LineSegment vertLs, LineSegment ls) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/segment/LineSegment.java` around lines 404 - 411, The file contains duplicate Javadoc blocks immediately above the method intersectionOneVertical; remove the stale/incomplete first Javadoc so only the intended comment ("Returns the intersection of one vertical line and another non-vertical line. Matches Clojure intersection-one-vertical.") remains directly above the private static Point intersectionOneVertical(LineSegment vertLs, LineSegment ls) declaration.
73-100: No validation thatp1andp2are distinct points.
createLineSegmentdoesn't check if both endpoints are the same point. A degenerate segment (zero length) would producevertical=true,horizontal=true, andm=POSITIVE_INFINITYwithb=null, which could cause unexpected behavior in intersection methods (e.g.,intersectionBothVerticalandintersectionBothHorizontalcould both be reached, but the dispatch inintersectionwould go to the vertical path first).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/segment/LineSegment.java` around lines 73 - 100, createLineSegment currently allows degenerate zero-length segments when p1 and p2 are identical; add a validation at the start of createLineSegment (using Point equality or comparing lon/lat from Point.getLon()/getLat()) to detect identical endpoints and fail fast (throw IllegalArgumentException with a clear message) instead of constructing a LineSegment, so the constructor/new LineSegment(...) is never invoked for identical points and downstream intersection branches cannot be ambiguously triggered.
210-241:densifyLineSegmentsilently returns only endpoints for vertical lines.For vertical lines, the method returns just the two endpoints ignoring
numPoints. This means callers expectingnumPoints + 1points will get only 2. If this is intentional (matching the Clojure implementation), consider documenting this behavior in the Javadoc.Add Javadoc note
/** * Creates intermediate points along the line segment for densification. + * Note: For vertical lines, only the two endpoints are returned regardless of numPoints. * * `@param` numPoints Number of intermediate points to create * `@return` List of points including endpoints and intermediate points */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/segment/LineSegment.java` around lines 210 - 241, densifyLineSegment currently returns only the two endpoints when the LineSegment is vertical (vertical flag) which breaks the expected contract of returning numPoints+1 points; either update densifyLineSegment to generate intermediate points for vertical segments by interpolating latitude between point1.getLat() and point2.getLat() while keeping longitude constant (use numPoints to compute lat step and produce indices 0..numPoints), or if the two-point behavior is intentional to match the Clojure impl, add a clear Javadoc on densifyLineSegment explaining that when vertical is true it returns only the endpoints and does not honor numPoints so callers know this special-case behavior.spatial-lib/src/java/cmr/spatial/geometry/PointMbrIntersections.java (2)
33-34: Snake_case method names deviate from Java conventions.Methods like
geodetic_covers_point,covers_lat, andgeodetic_lon_range_covers_lonuse snake_case, which is non-idiomatic for Java. This appears intentional to mirror the Clojure function names, which is reasonable for maintainability during migration — just flagging for awareness.Also applies to: 45-46, 73-73, 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/PointMbrIntersections.java` around lines 33 - 34, Rename the snake_case methods to Java camelCase equivalents and update all usages: change geodetic_covers_point to geodeticCoversPoint (and its overload), covers_lat to coversLat, and geodetic_lon_range_covers_lon to geodeticLonRangeCoversLon; update any calls, tests, and imports to the new names, and optionally add small deprecated wrappers with the old names that call the new methods if backwards compatibility is required. Ensure method signatures (Mbr, Point, MathUtils.COVERS_TOLERANCE overload) and visibility remain unchanged while only renaming identifiers.
109-118: Dead code: private helpersisNorthPoleandisSouthPoleare never called.These private methods delegate to
PointIntersections.isNorthPole/isSouthPole, butgeodetic_covers_pointon lines 50 and 55 already calls those methods directly. These wrappers are unused.Remove dead code
- /** - * Helper method: returns true if latitude is north pole. - */ - private static boolean isNorthPole(double lat, double delta) { - return PointIntersections.isNorthPole(lat, delta); - } - - /** - * Helper method: returns true if latitude is south pole. - */ - private static boolean isSouthPole(double lat, double delta) { - return PointIntersections.isSouthPole(lat, delta); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/PointMbrIntersections.java` around lines 109 - 118, Remove the two unused private helper methods isNorthPole(double lat, double delta) and isSouthPole(double lat, double delta) from PointMbrIntersections; they merely delegate to PointIntersections.isNorthPole/isSouthPole and are never referenced (geodetic_covers_point already calls PointIntersections directly). Delete both methods, run a build/tests to ensure no references remain, and remove any now-unused imports if the cleanup makes them redundant.spatial-lib/src/cmr/spatial/point.clj (1)
399-403: Missing import forcmr.spatial.shape.Point— works but inconsistent.Line 401 uses the fully-qualified constructor
cmr.spatial.shape.Point.without an explicit:importin thensform. While Clojure allows fully-qualified class construction, this is inconsistent with the explicit import ofPointIntersectionson line 17. Consider addingcmr.spatial.shape.Pointto the:importblock for clarity and consistency.Suggested import addition
(:import - [cmr.spatial.geometry PointIntersections])) + [cmr.spatial.geometry PointIntersections] + [cmr.spatial.shape Point]))Then update lines 401-402:
- (let [java-p1 (cmr.spatial.shape.Point. (.lon p1) (.lat p1)) - java-p2 (cmr.spatial.shape.Point. (.lon p2) (.lat p2))] + (let [java-p1 (Point. (.lon p1) (.lat p1)) + java-p2 (Point. (.lon p2) (.lat p2))]Note: You'd need to alias or qualify to avoid conflict with the Clojure
Pointdeftype in this same namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/point.clj` around lines 399 - 403, Add cmr.spatial.shape.Point to the ns :import so the Java Point is imported alongside PointIntersections, then update the constructor usage in the pointsApproxEqual implementation to use the imported Java class constructor (Point.) instead of the fully-qualified cmr.spatial.shape.Point.; if that import would collide with the Clojure Point deftype in this namespace, keep using the fully-qualified constructor or rename the local Clojure type to avoid the symbol conflict (affecting the function that delegates to PointIntersections/pointsApproxEqual and the variables p1/p2).spatial-lib/src/java/cmr/spatial/relations/ShapePredicate.java (1)
7-16: UseSpatialShapeinstead ofObjectfor the parameter type.All shape types (Point, Mbr, Polygon, LineString, Ring, Circle) implement
SpatialShape. Using it as the parameter type would provide compile-time type safety while clarifying that the Java API is intended only for shapes implementing this interface. Clojure users are already directed to use the Clojure API instead.Proposed change
+import cmr.spatial.shape.SpatialShape; + `@FunctionalInterface` public interface ShapePredicate { - boolean intersects(Object shape); + boolean intersects(SpatialShape shape); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/relations/ShapePredicate.java` around lines 7 - 16, The intersects method in the ShapePredicate functional interface currently accepts Object; change its parameter type to SpatialShape to enforce compile-time type safety for all shape implementations (Point, Mbr, Polygon, LineString, Ring, Circle). Update the method signature in ShapePredicate.intersects and its Javadoc param description to reference SpatialShape (and add an import if needed) so callers must pass objects implementing the SpatialShape interface.spatial-lib/src/cmr/spatial/line_string.clj (1)
153-182: Extract JavaLineStringconversion into one helper.The same Clojure→Java
LineStringconversion is duplicated across three methods; a shared helper will reduce maintenance drift.♻️ Suggested refactor
+(defn- ->java-line-string + [^LineString line] + (cmr.spatial.shape.LineString. + (name (.coordinate_system line)) + (vec (p/points->ords (.points line))))) + (defn covers-point? "Returns true if the line covers the point" [^LineString line ^cmr.spatial.point.Point point] ;; Delegate to Java implementation - (let [java-line (cmr.spatial.shape.LineString. - (name (.coordinate_system line)) - (vec (p/points->ords (.points line)))) + (let [java-line (->java-line-string line) java-point (cmr.spatial.shape.Point. (.lon point) (.lat point))] (LineStringIntersections/coversPoint java-line java-point)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/cmr/spatial/line_string.clj` around lines 153 - 182, The Clojure→Java LineString conversion is duplicated in covers-point?, intersects-br? and intersects-line-string?; create a single helper (e.g. ->java-line-string or convert-line-string) that accepts the Clojure LineString (or a map with :coordinate-system and :points) and returns a cmr.spatial.shape.LineString by reusing (name ...) and (vec (p/points->ords ...)); then replace the inline construction in covers-point?, intersects-br? and intersects-line-string? to call that helper, keeping the existing java-point and java-mbr constructions and the calls to LineStringIntersections unchanged.es-spatial-plugin/src/java/cmr/elasticsearch/plugins/SpatialSearchPlugin.java (1)
221-236: Unfinished helper should be completed or removed before merge.
getFieldsLookupis currently dead/incomplete (return null; // Will fix this properly), which leaves an ambiguous code path in a critical runtime component.If you want, I can draft a clean replacement using one field-access path and open a follow-up issue template for it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@es-spatial-plugin/src/java/cmr/elasticsearch/plugins/SpatialSearchPlugin.java` around lines 221 - 236, getFieldsLookup currently returns null and must be implemented or removed; implement it by invoking the reflected method getLeafSearchLookup on the existing searchLookup object (use the Method getLeafSearchLookup = searchLookup.getClass().getMethod("getLeafSearchLookup", org.apache.lucene.index.LeafReaderContext.class)) and pass a valid org.apache.lucene.index.LeafReaderContext obtained from the plugin’s per-document reader (e.g., unwrap the DocReader/FilterScript holder to get its LeafReaderContext or a DocLookup/DocReader instance that exposes the context), then cast the invocation result to LeafStoredFieldsLookup and return it; keep the existing try/catch, log and return null only on failure; alternatively, if no reliable way to obtain a LeafReaderContext exists in this class, remove getFieldsLookup and all callers to avoid a dangling dead path (referencing getFieldsLookup, searchLookup, getLeafSearchLookup, DocReader, FilterScript).spatial-lib/src/java/cmr/spatial/internal/arc/Arc.java (3)
383-392: Floating-point==for course comparisons incrossesNorthPole/crossesSouthPole.These methods use exact
==comparison ondoublevalues. This works only because the compared values originate fromcalculateCourse's special-case branches that return literal0.0/180.0, and the stored fields are never recomputed. This is fragile — if course computation ever changes to go through the general formula for near-pole arcs, these checks would silently fail. An approximate comparison would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/arc/Arc.java` around lines 383 - 392, Replace the fragile exact double comparisons in crossesNorthPole() and crossesSouthPole() with tolerant comparisons using a small epsilon: compare initialCourse and endingCourse to 0.0 and 180.0 with Math.abs(a - b) <= EPSILON (define a private static final double EPSILON = 1e-9 or similar) so near-equal results from calculateCourse() still register; update both methods to use this epsilon-based check and reference initialCourse, endingCourse, and calculateCourse in the comment or javadoc if helpful.
296-309: Non-standard bearing convention — verify this is intentional.The general-case bearing formula on line 308 uses
(-normalized + 360) % 360instead of the conventional(normalized + 360) % 360. This effectively computes the reverse azimuth convention (e.g., due east → 270° instead of 90°). The code appears to be internally consistent since the general-case course values are only used forequals/hashCodeand never compared against specific degree constants—pole detection is handled by the special-case branches above. However, this deviates from the reference formulary and could cause subtle issues if the courses are ever used directly downstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/arc/Arc.java` around lines 296 - 309, The computed bearing uses the reversed sign: change the normalization from using (-normalized + 360) % 360 to the standard (normalized + 360) % 360 so the azimuth follows the conventional clockwise-from-north degrees; update the expression that returns the bearing (the line that uses the local variable "normalized" in Arc.java) and ensure any downstream uses (e.g., where course values are used in equals/hashCode) remain correct after switching to the conventional azimuth direction.
230-243:compareLongitudesnever returns 0 when longitudes are truly equal.When
mod == 0.0(line 236), the method returns-1or1rather than0. This is guarded bycomparePointscheckinglon1 == lon2before calling this method, so it works in practice, but the method's own Javadoc claims it "Returns … 0 if equal." If this method is ever called directly with equal longitudes, it will give wrong results. Consider returning0formod == 0.0or updating the doc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/arc/Arc.java` around lines 230 - 243, The compareLongitudes method incorrectly never returns 0 for equal longitudes; change its logic so that when mod == 0.0 the function returns 0 (i.e., detect exact-equality first) instead of returning -1/1; keep the special-case for mod == 180.0 (use Double.compare(l1, l2) to break that tie) and ensure any callers like comparePoints still behave the same; alternatively, if you prefer the asymmetric tie-breaker behavior, update the Javadoc for compareLongitudes to remove the "0 if equal" claim—prefer the first option (return 0) to match the doc and expected semantics.spatial-lib/src/java/cmr/spatial/geometry/LineStringIntersections.java (1)
162-172: Silent swallowing of allIllegalArgumentExceptions may mask non-degenerate errors.The catch block at line 167 discards all
IllegalArgumentExceptions during arc creation. While degenerate points (equal/antipodal) are expected, other unexpected argument errors (e.g., NaN coordinates) would also be silently swallowed. Consider logging at debug level or narrowing the catch condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/LineStringIntersections.java` around lines 162 - 172, The pointsToArcs method currently swallows all IllegalArgumentException from Arc.createArc, hiding non-degenerate errors; update pointsToArcs to pre-validate pairs (e.g., check for equal or antipodal points) and skip only those predictable degenerate cases before calling Arc.createArc, or if you keep the try/catch, narrow handling so that IllegalArgumentException is inspected and only known-degenerate cases are ignored while other exceptions are either logged at debug/error (include e.getMessage()) or rethrown; refer to pointsToArcs and Arc.createArc when making this change.spatial-lib/src/java/cmr/spatial/relations/ShapeIntersections.java (2)
218-244: Inconsistent null guard — onlycreateCircleIntersectsFnchecks for nullotherShape.Lines 220–222 guard against
null, but the other five predicate factories (Point, Mbr, LineString, Ring, Polygon) will throw an unguarded NPE onnullinput. Either add the guard consistently to all predicates or remove it here to keep the contract uniform (e.g., document that callers must not pass null).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/relations/ShapeIntersections.java` around lines 218 - 244, createCircleIntersectsFn is the only predicate factory that guards against a null otherShape; make this consistent by adding the same null check and IllegalArgumentException("Intersection not implemented for null shape") to the other predicate factories (e.g., createPointIntersectsFn, createMbrIntersectsFn, createLineStringIntersectsFn, createRingIntersectsFn, createPolygonIntersectsFn) at the top of their returned ShapePredicate lambdas, so all implementations uniformly validate otherShape before performing instanceof checks and delegating to the respective *Intersections methods.
95-121:createMbrIntersectsFnrecreatesjavaRingon every call for Ring shapes.On line 107,
RingIntersections.createJavaRing((Ring) otherShape)is called each time the predicate is tested against a Ring. In contrast,createRingIntersectsFn(line 160) wisely pre-computesjavaRingonce. If this predicate is called frequently with Ring arguments (e.g., in an ES spatial scan), the repeated conversion could be costly. Consider caching or documenting that the hot path is expected to go throughcreateRingIntersectsFninstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/relations/ShapeIntersections.java` around lines 95 - 121, The predicate createMbrIntersectsFn currently calls RingIntersections.createJavaRing((Ring) otherShape) inside the lambda on every test, which is expensive; fix by mirroring createRingIntersectsFn’s approach: add a specialized factory (e.g., createMbrIntersectsFnForRing or overload of createMbrIntersectsFn) that accepts a Ring, calls RingIntersections.createJavaRing(ring) once to produce a cached javaRing, and returns a ShapePredicate that uses the precomputed javaRing in RingIntersections.intersectsBr(javaRing, mbr); update call sites to use this specialized factory when the Ring is known, or implement a small cache keyed by Ring if callers cannot be changed.spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java (3)
589-620: Duplicate geodetic covers-point / covers-lat / covers-lon logic across files.
geodeticCoversPoint,cartesianCoversPoint,coversLat,coversLonGeodetic, andcoversLonCartesianare duplicated nearly verbatim inArcLineSegmentIntersections, and similar logic exists inPointMbrIntersections. This creates a maintenance risk — a bug fix in one copy could be missed in others. Consider consolidating into a single utility (e.g.,MbrUtilsor extendingPointMbrIntersections).Also applies to: 645-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java` around lines 589 - 620, Duplicate point-in-MBR logic exists across geodeticCoversPoint, cartesianCoversPoint, coversLat, coversLonGeodetic, and coversLonCartesian (also in ArcLineSegmentIntersections and PointMbrIntersections); extract these methods into a single shared utility class (e.g., MbrUtils) or a common helper in PointMbrIntersections, move the implementations for coversLat, coversLonGeodetic, coversLonCartesian, geodeticCoversPoint, and cartesianCoversPoint there, update callers in RingIntersections, ArcLineSegmentIntersections, and PointMbrIntersections to call the new utility methods, keep the original method signatures or provide thin wrappers for binary compatibility, and run/update unit tests to ensure behavior is unchanged.
314-320:HashSetprovides no benefit here —containsApproximatePointdoes a linear scan.
acceptablePointsis aHashSet<Point>, butcontainsApproximatePointiterates over every element callingpointsApproxEqual. TheHashSetadd/contains semantics (based onequals/hashCode) are never leveraged. A plainArrayList(or evenPoint[]) would be clearer and avoid the hashing overhead.Proposed fix
- Set<Point> acceptablePoints = new HashSet<>(); - for (Point p : getRingPoints(ring)) { - acceptablePoints.add(p); - } - for (Point p : cornerPoints) { - acceptablePoints.add(p); - } + List<Point> acceptablePoints = new ArrayList<>(); + for (Point p : getRingPoints(ring)) { + acceptablePoints.add(p); + } + for (Point p : cornerPoints) { + acceptablePoints.add(p); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java` around lines 314 - 320, Replace the HashSet usage for acceptablePoints with a simple list/array because containsApproximatePoint performs a linear scan with pointsApproxEqual and never uses Point.equals/hashCode; specifically, in RingIntersections.java replace the Set<Point> acceptablePoints = new HashSet<>() and its .add calls (population from getRingPoints(ring) and cornerPoints) with an ArrayList<Point> (or Point[]) and keep the rest of the code that calls containsApproximatePoint unchanged, so you avoid unnecessary hashing overhead while preserving the approximate-equality checks.
429-516: Consider a common interface for GeodeticRing/CartesianRing to eliminate repeated instanceof dispatch.Five private helpers (
getSegments,getCoordinateSystem,getRingPoints,getRingMbr,ringCoversPoint) all repeat the sameinstanceof GeodeticRing / instanceof CartesianRing / else throwpattern. If both ring types implemented a shared interface (e.g.,SpatialRing) exposinggetSegments(),getCoordinateSystem(),getPoints(),getMbr(), andcoversPoint(Point), all five dispatchers collapse to single method calls and the public API can acceptSpatialRinginstead ofObject.This would also make the API type-safe at compile time, eliminating the runtime
IllegalArgumentExceptionpaths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/geometry/RingIntersections.java` around lines 429 - 516, The five helper methods (getSegments, getCoordinateSystem, getRingPoints, getRingMbr, ringCoversPoint) duplicate instanceof dispatch for GeodeticRing/CartesianRing; introduce a new interface (e.g., SpatialRing) declaring getSegments()/getCoordinateSystem()/getPoints()/getMbr()/coversPoint(Point) and have GeodeticRing and CartesianRing implement it, then replace those helpers to accept SpatialRing and simply call the interface methods (and update public API signatures to use SpatialRing instead of Object) so the runtime instanceof checks and IllegalArgumentException branches can be removed.spatial-lib/src/java/cmr/spatial/internal/arc/ArcLineSegmentIntersections.java (1)
407-453: Dead code and incomplete subselection in densification loop.Lines 415–422:
segmentToProcessis always assignedlsand never reassigned, making thep1In/p2Inchecks and the associatedif (!p1In && !p2In)block dead code with only a comment. The intent to clip the segment to the MBR intersection is noted but not implemented.This means the entire original line segment is always densified for every intersecting MBR, potentially doing redundant work when there are multiple arc MBRs (e.g., pole-crossing arcs with 2 MBRs). It's functionally correct but wasteful.
If clipping isn't planned for now, simplify by removing the dead variables and adding a TODO.
Simplified version
for (Mbr mbr : mbrIntersections) { - // Simple subselection: check if endpoints are in MBR - Point p1 = ls.getPoint1(); - Point p2 = ls.getPoint2(); - - boolean p1In = cartesianCoversPoint(mbr, p1); - boolean p2In = cartesianCoversPoint(mbr, p2); - - LineSegment segmentToProcess = ls; - - // If only one point is in, we might need to clip - // For simplicity, we'll process the whole segment if any part intersects - if (!p1In && !p2In) { - // Neither point in MBR, but MBR intersects - need to find intersection points - // This is complex, so we'll just densify the whole segment - } - - // Densify the line segment - List<Point> densifiedPoints = densifyLineSegment(segmentToProcess, DEFAULT_DENSIFICATION_DISTANCE); + // TODO: clip segment to MBR intersection for efficiency + List<Point> densifiedPoints = densifyLineSegment(ls, DEFAULT_DENSIFICATION_DISTANCE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/arc/ArcLineSegmentIntersections.java` around lines 407 - 453, The p1In/p2In checks and the segmentToProcess variable in the loop inside ArcLineSegmentIntersections are dead (segmentToProcess is never reassigned) — either implement clipping of ls to the MBR or remove the unused checks and variable; to fix, delete p1In, p2In and segmentToProcess (and the empty if (!p1In && !p2In) block) and add a short TODO comment referencing densifyLineSegment and Arc.createArc/arcArcIntersections to indicate where clipping should be implemented later, keeping the densification and arc intersection logic (densifyLineSegment, pointsApproxEqual, Arc.createArc, arcArcIntersections, arc.pointOnArc) unchanged.spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java (2)
706-722: Getters return mutable internal lists directly.
getPointsList()andgetArcsList()return the internalListreferences. Callers could mutate the ring's state. Consider returningCollections.unmodifiableList(...)or defensive copies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java` around lines 706 - 722, The getters getPointsList() and getArcsList() currently expose the internal mutable Lists (points, arcs); change them to return immutable views or defensive copies (e.g., wrap with Collections.unmodifiableList(...) or return new ArrayList<>(points)/new ArrayList<>(arcs)) so external callers cannot mutate GeodeticRing state; keep getPoints() and getArcs() behavior (they already return arrays) or mirror defensive-copy semantics if you prefer consistency.
88-108:convertToJavaPointsis duplicated verbatim inCartesianRing.java.Both
GeodeticRing.convertToJavaPoints(lines 88–108) andCartesianRing.convertToJavaPoints(lines 71–91 in CartesianRing.java) contain identical reflection-based Clojure interop logic. Consider extracting this into a shared utility (e.g., a static method onPointor aPointUtilsclass) to avoid the maintenance burden of keeping two copies in sync.Also applies to: 65-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/java/cmr/spatial/internal/ring/GeodeticRing.java` around lines 88 - 108, GeodeticRing.convertToJavaPoints and CartesianRing.convertToJavaPoints are duplicate reflection-based Clojure interop code; extract the conversion logic into a single shared method (either a static factory on Point such as Point.fromInterop(Object) or a PointUtils.convertToJavaPoints(List<?>) helper), move the reflection + exception handling there, and update both GeodeticRing.convertToJavaPoints and CartesianRing.convertToJavaPoints to delegate to that shared method; ensure the new shared method returns List<Point> and preserves the current behavior and error wrapping so callers need only call the single implementation.spatial-lib/src/test/java/cmr/spatial/geometry/IntersectionTestsTest.java (1)
9-204: Test coverage only covers point/MBR primitives — consider adding ring and arc-segment tests.This test file validates
PointIntersections,PointMbrIntersections, andMbrIntersectionswell, but the PR also introduces complex new classes (GeodeticRing,CartesianRing,RingIntersections,ArcLineSegmentIntersections) with no dedicated unit tests in this file. The AI summary mentionsESPluginJavaIntersectionsTestexists elsewhere, but targeted unit tests for the ring and arc-segment logic would help catch edge-case regressions independently.Would you like me to generate test cases for
GeodeticRing.coversPoint,CartesianRing.coversPoint, andArcLineSegmentIntersections.arcArcIntersections?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spatial-lib/src/test/java/cmr/spatial/geometry/IntersectionTestsTest.java` around lines 9 - 204, Add focused unit tests for the new ring and arc-segment classes: create tests for GeodeticRing.coversPoint and CartesianRing.coversPoint that cover points inside, outside, exactly on the ring boundary, at poles/antimeridian where applicable, and for rings that cross the antimeridian; and create tests for ArcLineSegmentIntersections.arcArcIntersections that cover intersecting arcs (single and two intersection points), tangent arcs (one shared point), non-intersecting arcs, colinear/overlapping arcs, and edge cases with endpoints on intersection; implement these as small, independent JUnit methods mirroring existing style and assertions in this file and reference the exact method names (GeodeticRing.coversPoint, CartesianRing.coversPoint, ArcLineSegmentIntersections.arcArcIntersections) so they run fast and isolate ring/arc logic from other integration tests.
| echo "root soft memlock unlimited" >> /etc/security/limits.conf &&\ | ||
| echo "root hard memlock unlimited" >> /etc/security/limits.conf |
There was a problem hiding this comment.
Memlock limits are configured for the wrong user.
Limits are appended for root, but Elasticsearch runs as UID 1000 here, so the intended memlock setting will not apply at runtime.
🔧 Proposed fix
- echo "root soft memlock unlimited" >> /etc/security/limits.conf &&\
- echo "root hard memlock unlimited" >> /etc/security/limits.conf
+ echo "elasticsearch soft memlock unlimited" >> /etc/security/limits.conf &&\
+ echo "elasticsearch hard memlock unlimited" >> /etc/security/limits.confAlso applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@es-spatial-plugin/Dockerfile` around lines 15 - 16, The Dockerfile is
appending memlock limits for "root" but Elasticsearch runs as UID 1000, so the
settings never apply; change the two echo lines that write "root soft memlock
unlimited" and "root hard memlock unlimited" to target the Elasticsearch runtime
user (UID 1000) or use a wildcard entry (e.g., "*" or the actual username used
at runtime) so the memlock limits apply to the process that runs Elasticsearch;
update the two instances in the Dockerfile where those echoes appear
accordingly.
| (defn get-list-of-dep-jars [] | ||
| (let [all-jars (into [] (map #(.getName %) (.listFiles (clojure.java.io/file "target/lib")))) | ||
| ;; Minimal set: only what spatial-lib Java code actually needs | ||
| allowed-prefixes ["clojure" ; Runtime (spatial-lib has compiled Clojure) | ||
| "cmr-spatial-lib" ; Main library | ||
| "jafama" ; Math library used by spatial calculations | ||
| "primitive-math" ; Math optimizations | ||
| "vectorz"]] ; Vector math library | ||
| (map #(str "target/lib/" %) (filter (fn [jar-name] (some (fn [prefix] (str/starts-with? jar-name prefix)) allowed-prefixes)) all-jars)))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the full project.clj file
find . -name "project.clj" -path "*/es-spatial-plugin/*" | head -1 | xargs cat -nRepository: nasa/Common-Metadata-Repository
Length of output: 9514
🏁 Script executed:
# Also check for any require or ns declarations at the top of the file
head -50 es-spatial-plugin/project.cljRepository: nasa/Common-Metadata-Repository
Length of output: 2425
🏁 Script executed:
# Search for any aliasing of clojure.string or str namespace in the file
rg -n "(require|ns|:as str)" es-spatial-plugin/project.cljRepository: nasa/Common-Metadata-Repository
Length of output: 1134
str/starts-with? is unresolved — replace with fully-qualified namespace.
Line 37 uses str/starts-with?, but str is not aliased. All other string operations in this file use clojure.string/... with full qualification (lines 4, 7-9). The :clj-kondo/ignore [:unresolved-namespace] directive on line 1 explicitly suppresses this warning, confirming the issue is known.
Replace line 37:
Fix
- (map #(str "target/lib/" %) (filter (fn [jar-name] (some (fn [prefix] (str/starts-with? jar-name prefix)) allowed-prefixes)) all-jars))))
+ (map #(str "target/lib/" %) (filter (fn [jar-name] (some (fn [prefix] (clojure.string/starts-with? jar-name prefix)) allowed-prefixes)) all-jars))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@es-spatial-plugin/project.clj` around lines 29 - 37, The unresolved use of
str/starts-with? inside get-list-of-dep-jars should be replaced with the
fully-qualified clojure.string/starts-with?; update the filter callback that
checks jar-name against allowed-prefixes to call clojure.string/starts-with? (or
require alias) so the referenced symbol is resolved and consistent with other
string usage around allowed-prefixes and get-list-of-dep-jars.
| "package-es-plugin" ~(vec (concat ["do" | ||
| ["shell" "echo" "Packaging ES plugin into zip file"] | ||
| "shell" | ||
| "zip" | ||
| "-j" | ||
| plugin-zip-name | ||
| plugin-jar-name | ||
| "resources/plugin/plugin-descriptor.properties"] | ||
| (get-list-of-dep-jars))) |
There was a problem hiding this comment.
get-list-of-dep-jars is evaluated at project load time — jar list may be empty.
The ~(vec (concat ...)) unquote on line 113 evaluates (get-list-of-dep-jars) when project.clj is read, not when the package-es-plugin alias is executed. If target/lib doesn't exist yet (e.g., before gather-dependencies has ever been run), the jar list will be empty, and the zip will be missing dependency jars.
This means prepare-es-plugin and package-es-plugin must be run as separate Leiningen invocations (so project.clj is re-read after target/lib is populated). Running them in a single lein do prepare-es-plugin, package-es-plugin won't work correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@es-spatial-plugin/project.clj` around lines 113 - 121, The alias
package-es-plugin currently unquotes ~(vec (concat ... (get-list-of-dep-jars)))
which evaluates get-list-of-dep-jars at project read time and can yield an empty
list; change package-es-plugin so the dependency jars are resolved at runtime
instead of when project.clj is loaded — for example replace the evaluated vector
with a runtime shell command (keep the "do" steps but use a single "shell" that
runs bash -lc and globs target/lib/*.jar, e.g. "bash -lc 'zip -j
<plugin-zip-name> <plugin-jar-name>
resources/plugin/plugin-descriptor.properties target/lib/*.jar'"), or implement
package-es-plugin as a small Leiningen task function that calls
get-list-of-dep-jars at invocation time; adjust references to package-es-plugin
and get-list-of-dep-jars accordingly.
| java.version=11 | ||
| # | ||
| # 'elasticsearch.version': version of elasticsearch compiled against | ||
| elasticsearch.version=7.17.14 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the files mentioned in the review exist and examine their contents
find . -type f -path "*es-spatial-plugin/resources/plugin/plugin-descriptor.properties" 2>/dev/null | head -5Repository: nasa/Common-Metadata-Repository
Length of output: 140
🏁 Script executed:
# Search for version declarations in key plugin files
rg -n "java\.version|elasticsearch\.version" --type properties 2>/dev/null | head -20Repository: nasa/Common-Metadata-Repository
Length of output: 57
🏁 Script executed:
# Check the Dockerfile for Elasticsearch version
find . -type f -name "Dockerfile" -path "*es-spatial-plugin*" 2>/dev/null | xargs cat 2>/dev/nullRepository: nasa/Common-Metadata-Repository
Length of output: 907
🏁 Script executed:
# Check project.clj or build files for version declarations
find . -type f -path "*es-spatial-plugin*" \( -name "project.clj" -o -name "*.gradle" -o -name "pom.xml" \) 2>/dev/null | xargs ls -lh 2>/dev/nullRepository: nasa/Common-Metadata-Repository
Length of output: 155
🏁 Script executed:
cat -n ./es-spatial-plugin/resources/plugin/plugin-descriptor.propertiesRepository: nasa/Common-Metadata-Repository
Length of output: 794
🏁 Script executed:
cat -n ./es-spatial-plugin/project.cljRepository: nasa/Common-Metadata-Repository
Length of output: 9514
Update plugin descriptor Elasticsearch version to match build target.
Line 19 declares elasticsearch.version=7.17.14 but this PR builds against Elasticsearch 8.18.7 (as specified in project.clj line 52 and the Dockerfile). Elasticsearch enforces version compatibility at plugin install time, so the mismatch will cause the plugin to fail loading.
Update the descriptor to match:
Fix
-elasticsearch.version=7.17.14
+elasticsearch.version=8.18.7📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| java.version=11 | |
| # | |
| # 'elasticsearch.version': version of elasticsearch compiled against | |
| elasticsearch.version=7.17.14 | |
| java.version=11 | |
| # | |
| # 'elasticsearch.version': version of elasticsearch compiled against | |
| elasticsearch.version=8.18.7 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@es-spatial-plugin/resources/plugin/plugin-descriptor.properties` around lines
16 - 19, Update the plugin descriptor property elasticsearch.version in
plugin-descriptor.properties to match the build target (Elasticsearch 8.18.7);
locate the existing line "elasticsearch.version=7.17.14" and change its value to
"8.18.7" so the plugin descriptor aligns with project.clj/Dockerfile and avoids
install-time compatibility failures.
| SpatialShape queryShape = shapes.get(0); | ||
| ShapePredicate intersectsFn = ShapeIntersections.createIntersectsFn(queryShape); | ||
|
|
||
| return new SpatialLeafFactory(intersectsFn, lookup); |
There was a problem hiding this comment.
Using only shapes.get(0) silently drops additional query shapes.
If multiple top-level query shapes are provided, this code ignores all but the first one.
🐛 Minimal safety fix
- SpatialShape queryShape = shapes.get(0);
- ShapePredicate intersectsFn = ShapeIntersections.createIntersectsFn(queryShape);
+ if (shapes.size() != 1) {
+ throw new IllegalArgumentException(
+ "Expected exactly one query shape, got " + shapes.size());
+ }
+ ShapePredicate intersectsFn = ShapeIntersections.createIntersectsFn(shapes.get(0));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SpatialShape queryShape = shapes.get(0); | |
| ShapePredicate intersectsFn = ShapeIntersections.createIntersectsFn(queryShape); | |
| return new SpatialLeafFactory(intersectsFn, lookup); | |
| if (shapes.size() != 1) { | |
| throw new IllegalArgumentException( | |
| "Expected exactly one query shape, got " + shapes.size()); | |
| } | |
| ShapePredicate intersectsFn = ShapeIntersections.createIntersectsFn(shapes.get(0)); | |
| return new SpatialLeafFactory(intersectsFn, lookup); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@es-spatial-plugin/src/java/cmr/elasticsearch/plugins/SpatialSearchPlugin.java`
around lines 106 - 109, The code currently only uses shapes.get(0) which drops
any additional supplied SpatialShape instances; update the logic that builds the
ShapePredicate so it handles all entries in the shapes list instead of only the
first. Iterate over shapes, create a ShapePredicate for each via
ShapeIntersections.createIntersectsFn(SpatialShape) and compose them into a
single predicate that returns true if any individual predicate matches (logical
OR), then pass that composed predicate into the SpatialLeafFactory constructor
(replacing the single intersectsFn). Ensure you reference shapes,
ShapeIntersections.createIntersectsFn(...), ShapePredicate, and
SpatialLeafFactory when making the change.
| private static Mbr computeMbrIntersection(Mbr mbr1, Mbr mbr2) { | ||
| if (!MbrIntersections.mbrsIntersect("geodetic", mbr1, mbr2)) { | ||
| return null; | ||
| } | ||
|
|
||
| double west = Math.max(mbr1.getWest(), mbr2.getWest()); | ||
| double east = Math.min(mbr1.getEast(), mbr2.getEast()); | ||
| double north = Math.min(mbr1.getNorth(), mbr2.getNorth()); | ||
| double south = Math.max(mbr1.getSouth(), mbr2.getSouth()); | ||
|
|
||
| return new Mbr(west, north, east, south); | ||
| } |
There was a problem hiding this comment.
computeMbrIntersection can produce an invalid MBR when two MBRs intersect only at a pole.
Both inputs are guaranteed non-crossing (Arc splits, LS never crosses), and the comment says the Math.max/Math.min computation is safe. However, mbrsIntersect("geodetic", ...) returns true when two MBRs share only a pole touch (disjoint longitude ranges, both touching 90° N). In that case:
west = Math.max(w1, w2)andeast = Math.min(e1, e2)will producewest > east(an antimeridian-crossing MBR), which is semantically incorrect for a pole-only intersection.
This MBR is then used for cartesianCoversPoint checks in the densification loop, which doesn't handle crossing MBRs — so the subselection logic would silently break.
Consider returning null (or a degenerate point MBR at the pole) when the computed west > east or south > north.
Proposed guard
double west = Math.max(mbr1.getWest(), mbr2.getWest());
double east = Math.min(mbr1.getEast(), mbr2.getEast());
double north = Math.min(mbr1.getNorth(), mbr2.getNorth());
double south = Math.max(mbr1.getSouth(), mbr2.getSouth());
+ // Guard against degenerate intersection (e.g., pole-only touch with disjoint lon ranges)
+ if (west > east || south > north) {
+ return null;
+ }
+
return new Mbr(west, north, east, south);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spatial-lib/src/java/cmr/spatial/internal/arc/ArcLineSegmentIntersections.java`
around lines 472 - 483, computeMbrIntersection can return an invalid crossing
MBR when two MBRs only touch at a pole; after computing west/east/north/south in
computeMbrIntersection, detect the degenerate case where west > east or south >
north and handle it by returning null (or a degenerate Mbr representing the
single pole point) instead of constructing a crossing Mbr. Update the logic
around computeMbrIntersection (which currently calls
MbrIntersections.mbrsIntersect("geodetic", ...)) so callers that expect
non-crossing MBRs (e.g., code using cartesianCoversPoint in the densification
loop) receive null/degenerate MBRs for pole-only intersections and avoid
treating them as valid covering boxes.
| public static List<SpatialShape> ordsInfoToShapes(List<?> ordsInfo, List<?> ords) { | ||
| // Handle nil case - granules without spatial data | ||
| if (ordsInfo == null) { | ||
| return new ArrayList<>(); | ||
| } | ||
| if (ordsInfo.size() % 2 != 0) { | ||
| throw new IllegalArgumentException( | ||
| String.format("ordsInfo must contain pairs of (type,size), got odd length: %d. Contents: %s", | ||
| ordsInfo.size(), ordsInfo)); | ||
| } | ||
|
|
||
| List<SpatialShape> shapes = new ArrayList<>(); | ||
| List<Double> ordinates = convertToDoubleList(ords); | ||
| int ordsIndex = 0; |
There was a problem hiding this comment.
NPE if ords is null but ordsInfo is non-null.
Line 43 documents that ords "may be null," but line 58 passes ords directly to convertToDoubleList, which iterates over it unconditionally (line 150). If ordsInfo has entries while ords is null, this throws a NullPointerException. Either add an early null check for ords (returning an empty list or throwing a descriptive error) or correct the Javadoc to state ords must not be null when ordsInfo is non-null.
🐛 Proposed fix: guard against null ords
public static List<SpatialShape> ordsInfoToShapes(List<?> ordsInfo, List<?> ords) {
// Handle nil case - granules without spatial data
if (ordsInfo == null) {
return new ArrayList<>();
}
+ if (ords == null) {
+ throw new IllegalArgumentException(
+ "ords must not be null when ordsInfo is non-null. ordsInfo: " + ordsInfo);
+ }
if (ordsInfo.size() % 2 != 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spatial-lib/src/java/cmr/spatial/serialize/OrdsInfoShapes.java` around lines
46 - 59, ordsInfoToShapes currently calls convertToDoubleList(ords) without
checking ords, causing an NPE when ordsInfo is non-null but ords is null; add a
guard before the convertToDoubleList call in ordsInfoToShapes to detect if ords
== null while ordsInfo.size() > 0 and throw a clear IllegalArgumentException (or
alternatively return an empty list if that matches semantics) with a descriptive
message referencing ords and ordsInfo; update the method Javadoc to state that
ords must be non-null when ordsInfo is non-empty if you choose to require it.
| public Circle(Point center, double radius) { | ||
| this.center = center; | ||
| this.radius = radius; | ||
| } |
There was a problem hiding this comment.
Validate circle invariants at construction time.
Line [11]-[14] accepts null center and negative radius, allowing invalid circles into intersection code paths.
Proposed fix
package cmr.spatial.shape;
+import java.util.Objects;
+
/**
* Java representation of a Circle shape.
* A circle is defined by a center point and a radius in meters.
*/
public class Circle implements SpatialShape {
@@
public Circle(Point center, double radius) {
- this.center = center;
+ this.center = Objects.requireNonNull(center, "center must not be null");
+ if (radius < 0) {
+ throw new IllegalArgumentException("radius must be >= 0");
+ }
this.radius = radius;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Circle(Point center, double radius) { | |
| this.center = center; | |
| this.radius = radius; | |
| } | |
| public Circle(Point center, double radius) { | |
| this.center = Objects.requireNonNull(center, "center must not be null"); | |
| if (radius < 0) { | |
| throw new IllegalArgumentException("radius must be >= 0"); | |
| } | |
| this.radius = radius; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spatial-lib/src/java/cmr/spatial/shape/Circle.java` around lines 11 - 14, The
Circle constructor currently allows null center and negative radius; update the
Circle(Point center, double radius) constructor to validate invariants by
ensuring center is non-null (use Objects.requireNonNull or explicit check) and
radius is non-negative, and throw an IllegalArgumentException with a clear
message if validation fails so invalid Circle instances (fields center, radius)
cannot be created.
| if (onAntimeridian() && otherPoint.onAntimeridian()) { | ||
| return Math.abs(lat - otherPoint.lat) < APPROXIMATION_DELTA; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "*/Point.java" | head -5Repository: nasa/Common-Metadata-Repository
Length of output: 124
🏁 Script executed:
cat spatial-lib/src/java/cmr/spatial/shape/Point.java | head -150Repository: nasa/Common-Metadata-Repository
Length of output: 4477
🏁 Script executed:
# Get line count and then focus on the relevant sections
wc -l spatial-lib/src/java/cmr/spatial/shape/Point.javaRepository: nasa/Common-Metadata-Repository
Length of output: 128
Antimeridian branch violates equals/hashCode contract.
Line 99-101 uses raw delta comparison for latitude (Math.abs(lat - otherPoint.lat) < APPROXIMATION_DELTA), but line 124-125 hashes normalized latitude (Double.hashCode(normalize(lat))). This allows two equal points to have different hash codes, breaking HashMap/HashSet behavior and violating Java's equals/hashCode contract.
Example: Points with latitudes 0.6e-9 and -0.3e-9 on the antimeridian are considered equal (delta=0.9e-9 < 1e-9), but normalize produces 1e-9 and 0 respectively, resulting in different hash codes.
Align equals with hashCode by using normalized comparison:
Proposed fix
// -180 and 180 are considered equivalent longitudes
if (onAntimeridian() && otherPoint.onAntimeridian()) {
- return Math.abs(lat - otherPoint.lat) < APPROXIMATION_DELTA;
+ return normalize(lat) == normalize(otherPoint.lat);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (onAntimeridian() && otherPoint.onAntimeridian()) { | |
| return Math.abs(lat - otherPoint.lat) < APPROXIMATION_DELTA; | |
| } | |
| if (onAntimeridian() && otherPoint.onAntimeridian()) { | |
| return normalize(lat) == normalize(otherPoint.lat); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spatial-lib/src/java/cmr/spatial/shape/Point.java` around lines 99 - 101, The
antimeridian branch in Point.equals uses raw delta comparison on lat but
hashCode hashes normalize(lat), causing equals/hashCode mismatch; update the
antimeridian equality check in Point.equals (the block using onAntimeridian())
to compare normalized latitudes instead—e.g., compute normalize(lat) and
normalize(otherPoint.lat) and test Math.abs(normalize(lat) -
normalize(otherPoint.lat)) < APPROXIMATION_DELTA—so equals and
Double.hashCode(normalize(...)) remain consistent.
| public List<Ring> getHoles() { | ||
| return rings.size() <= 1 ? Collections.emptyList() : rings.subList(1, rings.size()); |
There was a problem hiding this comment.
getHoles() leaks mutable internal state.
Line [40] returns a live subList, so callers can mutate polygon internals (clear, add, etc.) through the returned value.
Proposed fix
public List<Ring> getHoles() {
- return rings.size() <= 1 ? Collections.emptyList() : rings.subList(1, rings.size());
+ if (rings.size() <= 1) {
+ return Collections.emptyList();
+ }
+ return Collections.unmodifiableList(new ArrayList<>(rings.subList(1, rings.size())));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spatial-lib/src/java/cmr/spatial/shape/Polygon.java` around lines 39 - 40,
getHoles() currently returns a live subList of the internal rings list which
allows callers to mutate internal state; to fix, change getHoles() so it returns
an immutable copy instead of rings.subList(1, rings.size()) — e.g. when
rings.size() > 1 create a new ArrayList from rings.subList(1, rings.size()) and
return either that copy or wrap it with Collections.unmodifiableList; update the
return in getHoles() and keep the rings field untouched.
1 participant
Overview
What is the objective?
I'm providing a rewritten ES spatial plugin implementation in Java only that depends on the Java spatial-lib implementation in #2383. We're still using leiningen to manage the build. This is tested on ES 8.18.7, and it appears to work (in that ES doesn't crash and spatial searches return intersecting geometry).
Providing this as supporting material for the spatial-lib rewrite for visibility to our NASA friends.
What are the changes?
The ES plugin was replaced by an all-java implementation.
What areas of the application does this impact?
search, es-spatial-plugin
Required Checklist
Additional Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements