Repository navigation
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 145 |
🟢 Coverage 76.98% diff coverage · +0.26% coverage variation
Metric Results Coverage variation ✅ +0.26% coverage variation (-1.00%) Diff coverage ✅ 76.98% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (ddd0eda) 92022 23214 25.23% Head commit (945b8e9) 92495 (+473) 23570 (+356) 25.48% (+0.26%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#3212) 543 418 76.98% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
dd79aee to
dfb20f9
Compare
| // geoField maps a KQL geo key to its indexed geopoint sibling, e.g. | ||
| // "location" to "location_geopoint". | ||
| func geoField(key string) string { | ||
| return strings.ToLower(key) + mapping.GeopointSuffix |
There was a problem hiding this comment.
Uh, this is bad. Should do a proper lookup of the fieldname. This currently works, as we only have the location - but not in the generic case
4629265 to
ada7162
Compare
a7ed42f to
e5c12a9
Compare
e4724ac to
9503884
Compare
ada7162 to
0033236
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The geo parity README table includes a truncated/mismatched GEO-09 query, which makes the added documentation incorrect.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds geo-spatial predicates to the KQL extension used by OpenCloud search, wiring them through parsing/AST, backend compilation (Bleve + OpenSearch), and parity coverage so geo filters behave consistently across engines.
Changes:
- Extend KQL grammar + AST with
geo.distance,geo.bbox, andgeo.polygonpredicates and shared parsing utilities. - Compile/transpile geo predicates to Bleve geo queries and OpenSearch geo_distance/geo_bounding_box/geo_shape queries, including geo-field resolution via mapping overrides.
- Add parity fixtures/tests plus unit tests for KQL parsing and OpenSearch query builder output; document geo query syntax.
File summaries
| File | Description |
|---|---|
| services/search/README.md | Documents new geo predicate syntax and composition rules in KQL. |
| services/search/pkg/query/resolver.go | Adds ResolveGeoField to map KQL keys to indexed geopoint sibling fields. |
| services/search/pkg/query/bleve/compiler.go | Compiles geo AST nodes into Bleve geo queries. |
| services/search/pkg/parity/README.md | Adds parity documentation section for geo query cases. |
| services/search/pkg/parity/query_geo_test.go | Introduces geo parity fixtures and cases for cross-engine validation. |
| services/search/pkg/parity/parity_test.go | Registers the new geo parity group. |
| services/search/pkg/opensearch/internal/osu/query_geo.go | Adds OpenSearch geo query builder types (distance/bbox/polygon). |
| services/search/pkg/opensearch/internal/osu/query_geo_test.go | Unit tests for OpenSearch geo query builders’ emitted maps. |
| services/search/pkg/opensearch/internal/osu/osu_suite_test.go | Ginkgo suite bootstrap for the OSU geo tests. |
| services/search/pkg/opensearch/internal/convert/kql_transpile.go | Transpiles geo AST nodes into the new OSU geo builders with geo-field validation. |
| services/search/pkg/opensearch/internal/convert/kql_transpile_test.go | Adds transpiler coverage for geo nodes and geo-on-non-geo rejection. |
| pkg/kql/geo.go | Adds parsing helpers for radius units, distance args, bbox args, and polygon vertices. |
| pkg/kql/geo_test.go | Adds Ginkgo tests validating parsing + normalization + composition/rejection behavior. |
| pkg/kql/geo_suite_test.go | Ginkgo suite bootstrap for KQL geo tests. |
| pkg/kql/factory.go | Adds factory builders to create geo AST nodes from parsed grammar actions. |
| pkg/kql/error.go | Introduces GeoFieldError and classifies it as a validation (bad request) error. |
| pkg/kql/dictionary.peg | Extends the PEG grammar to recognize geo predicates as property restrictions. |
| pkg/kql/dictionary_gen.go | Regenerates the parser to include geo predicate productions/actions. |
| pkg/ast/ast.go | Adds geo node types (GeoDistanceNode, GeoBoundingBoxNode, GeoPolygonNode) and key extraction support. |
Review details
- Files reviewed: 18/19 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f86b7d3 to
e51a304
Compare
|
hm, just to get the terminology right: when adding these operators we are moving from Keyword Query Language (KeyQL) to FAST Query Language (FQL), specifically what MS calls FQL operators FQL has no geo operations ... but sharepoint has doc on how to rank results based on the 3-D Euclidian distance: https://learn.microsoft.com/en-us/sharepoint/dev/general-development/sorting-search-results-in-sharepoint#examples
yeah ... right 🤪 I'm ok with extending our KQL parser ... But the README should mention the deviation and give an explanation ... which could point to an ADR if it gets too long winded. |
c52f30d to
3ba7a03
Compare
|
Introduced a specific KQL extensions section in the README. I would like to add a separate page with our extensions, limitations, examples,... when everything has settled a bit, but not now |
Extends the KQL grammar with geo function values on any field, e.g. location:geo.distance(lat, lon, 5km). Parses into GeoDistance/GeoBoundingBox/ GeoPolygon AST nodes; radius units (km/m/mi) are normalised to meters.
Translates the geo AST nodes to bleve GeoDistance/GeoBoundingBox/ GeoBoundingPolygon queries over the indexed <field>_geopoint sibling.
Adds geo_distance/geo_bounding_box query builders plus a geo_shape polygon query (relation intersects, the modern replacement for the deprecated geo_polygon, still valid against geo_point). Wires the geo AST nodes through the KQL transpiler onto the <field>_geopoint sibling.
…n-geo fields Replaces the ad-hoc key+"_geopoint" concatenation with a proper lookup (query.ResolveGeoField) derived from the TypeGeopoint field overrides, so it works for any geopoint field, not just location. Geo predicates on non-geopoint fields now error instead of querying a nonexistent field.
…haviour Sorts the two latitudes in the KQL layer so MinLat <= MaxLat. Only bleve strictly requires it (it errors on an inverted box; OpenSearch tolerates it), but doing it centrally keeps behaviour consistent across backends and the AST easy to reason about. Longitude order is preserved so an antimeridian-crossing box (minLon > maxLon) still works, which both backends handle identically.
3ba7a03 to
945b8e9
Compare
This is a non standard extension to KQL. If you want an ADR first, let me know...
Adds geo predicates to KQL, usable on any geopoint field wherever KQL is accepted (graph search, webdav search REPORT, ...):
location:geo.distance(lat, lon, 5km)(radius units km/m/mi)location:geo.bbox(minLat, minLon, maxLat, maxLon)location:geo.polygon(lat lon, lat lon, ...)They compose in the boolean tree, e.g.
mediatype:image AND location:geo.distance(48.2, 16.3, 5km). No proto, graph-endpoint or mapping change: thelocation_geopointindex sibling already comes from #3345, and the filter rides in the query string.Stacked on #3345 (reflection-based search mapping). KQL grammar/AST/parse, bleve compiler and opensearch transpiler, geo field resolution via mapping overrides.
No sort-by-distance: KQL only filters, that would need a structured request param.