Skip to content

Conversation

craigtaverner
Copy link
Contributor

@craigtaverner craigtaverner commented Aug 26, 2025

In the work at #129581 we added new geo-grid types: GEOHASH, GEOTILE and GEOHEX, as well as support functions for creating these from geo_point, long and keyword.

However, one of the key use cases we wish to replicate from the Query DSL is the grid search, and this involves the need to include the grid id inside a search predicate, in particular the ST_INTERSECTS function. For example:

FROM airports
| WHERE ST_INTERSECTS(location, TO_GEOTILE("3/4/3"))
| STATS
    count = COUNT(*),
    centroid = ST_CENTROID_AGG(location)

craigtaverner and others added 30 commits June 17, 2025 18:56
Added EsqlCapabily to block older BWC
Changed license IT to new API
The previous support kept the underlying IntBlock, which later would cause exceptions since the type expected a LongBock.
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

80% review here

) Expression left,
@Param(
name = "geomB",
type = { "geo_point", "cartesian_point", "geo_shape", "cartesian_shape", "geohash", "geotile", "geohex" },
Copy link
Contributor

Choose a reason for hiding this comment

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

The capability is in snapshot mode, but the docs will be productive, as well as the type resolution. Is that right? If they will all be snapshot, should we add some snapshot check in resolveType()? (I didn't see it at least). Or maybe set it as preview instead of snapshot, and add an appliesTo to the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we had a discussion about this in the previous PR. Making the docs generation snapshot aware is a much bigger lift. The previous PR leaks the types into several functions (TO_STRING, TO_LONG, and TO_GEOSHAPE in particular, but many more). The plan is to move this out of snapshot as soon as this new ST_INTERSECTS PR is merged (and give a couple of days to bake).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have applies_to for functions, but not for types. This is a subtle issue really, and we should discuss whether we should invest in applies_to support, or even snapshot support, for types. Right now neither option exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would an applies_to with preview (Or any other tag) work here, even if it's not "snaspshot" explicitly (We can add it in the description (?)).
Thinking not just about the snapshot period, but about the next release, as we'll have to add an appliesTo for the new version anyway (If I'm understanding the new docs correctly)

Copy link
Contributor

github-actions bot commented Sep 2, 2025

ℹ️ Important: Docs version tagging

👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version.

We use applies_to tags to mark version-specific features and changes.

Expand for a quick overview

When to use applies_to tags:

✅ At the page level to indicate which products/deployments the content applies to (mandatory)
✅ When features change state (e.g. preview, ga) in a specific version
✅ When availability differs across deployments and environments

What NOT to do:

❌ Don't remove or replace information that applies to an older version
❌ Don't add new information that applies to a specific version without an applies_to tag
❌ Don't forget that applies_to tags can be used at the page, section, and inline level

🤔 Need help?

}
}

protected boolean compareGeometryAndGrid(Geometry geometry, long gridId, DataType gridType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should this method be abstract and implemented in the specific classes? Both to move special logics out of this base class, and to avoid forgetting it (Or not finding the places to update) when adding grid to WITHIN and CONTAINS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a nice idea. But at the moment only CONTAINS has a concrete sub-class, and all others (INTERSECTS, DISJOINT and WITHIN) are operating entirely in the base class. This is because CONTAINS has some special non-standard behaviour forced on us by Lucene. We could create new sub-classes specifically for dealing with the new geo-grids (and therefor remove one switch statement for the grids code path), but I think the performance optimization is negligable at this point. For performance, the best value for money would be to support Lucene pushdown. Perhaps once I add support for CONTAINS/WITHIN or shapes, it will make the move to multiple sub-classes more worth while?

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have the subclasses (SpatialIntersects and SpatialDisjoint, as well as the others). I was thinking about moving the method there, to have all the logic related with each of them in the functions.
Are you talking about other subclasses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this method compareGeometryAndGrid is in the SpatialRelatesFunction class which has only one subclass for CONTAINS. It does not have a reference to the original SpatialIntersects or other function classes, so cannot call methods on that. I could think of two ways of getting a function into those classes, either making it static (in which case we still need a switch here) or by passing a reference down into this class. Both approaches seem a little too complex. I think I'll defer considering options here until we support either shapes or contains/within, which might motivate for a refactoring, but even then I suspect the current approach might still be a little cleaner.

Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

LGTM!

}
}

protected boolean compareGeometryAndGrid(Geometry geometry, long gridId, DataType gridType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have the subclasses (SpatialIntersects and SpatialDisjoint, as well as the others). I was thinking about moving the method there, to have all the logic related with each of them in the functions.
Are you talking about other subclasses?

@craigtaverner craigtaverner merged commit 473406d into elastic:main Sep 5, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL :Analytics/Geo Indexing, search aggregations of geo points and shapes >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants