Skip to content

Conversation

@craigtaverner
Copy link
Contributor

@craigtaverner craigtaverner commented Jun 30, 2025

The PR at #125143 was creating a new object per record for bounded geo-hex grids, due to failures on serverless that appeared to be caused by race conditions in shared memory. It turns out the ES|QL evaluators already have a solution for this, so this PR fixes it to create a new object per evaluator, and the driver already ensures that each evaluator-tree is only used by either a single thread, or multiple threads in series, and never concurrently.

@craigtaverner craigtaverner added Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >tech debt :Analytics/ES|QL AKA ESQL v9.2.0 labels Jun 30, 2025
@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.


public GeoHashBoundedGrid(int precision, GeoBoundingBox bbox) {
private GeoHashBoundedGrid(int precision, GeoBoundingBox bbox) {
this.precision = checkPrecisionRange(precision);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be removed since it is checked in the factory earlier

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

LGTM

Also fix an issue around nulls for out-of-bounds results

And investigate and affirm that the use of -1 internally to mark invalid grid-ids is fine for all three grid types (ie. -1 is not a valid grid-id in any of these cases, for different reasons in each).
@craigtaverner craigtaverner merged commit 27e02a9 into elastic:main Jul 3, 2025
32 checks passed
mridula-s109 pushed a commit to mridula-s109/elasticsearch that referenced this pull request Jul 3, 2025
…#130343)

Use new objects per thread instead of per record
* Expand tests to use random bounds, after multi-thread fix
* Also fix an issue around nulls for out-of-bounds results
* And investigate and affirm that the use of -1 internally to mark invalid grid-ids is fine for all three grid types (ie. -1 is not a valid grid-id in any of these cases, for different reasons in each).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue 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.

3 participants