Skip to content

[8.19] Ensure ordinal builder emit ordinal blocks (#127949) #127979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 20, 2025

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented May 9, 2025

Backports the following commits to 8.19:

@dnhatn dnhatn added :Analytics/ES|QL AKA ESQL >bug auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels May 9, 2025
@dnhatn dnhatn force-pushed the backport/8.19/pr-127949 branch 2 times, most recently from 2742974 to 2015e37 Compare May 20, 2025 05:17
Currently, if a field has high cardinality, we may mistakenly disable
emitting ordinal blocks. For example, with 10,000 `tsid` values, we
never emit ordinal blocks during reads, even though we could emit blocks
for 10 `tsid` values across 1,000 positions. This bug disables
optimizations for value aggregation and block hashing.

This change tracks the minimum and maximum seen ordinals and uses them
as an estimate for the number of ordinals. However, if a page contains
`ord=1` and `ord=9999`, ordinal blocks still won't be emitted.
Allocating a bitset or an array for `value_count` could track this more
accurately but would require additional memory. I need to think about
this trade off more before opening another PR to fix this issue
completely.

This is a quick, contained fix that significantly speeds up time-series
aggregation (and other queries too).

The execution time of this query is reduced from 3.4s to 1.9s with 11M documents.

```
POST /_query
{
    "profile": true,
    "query": "TS metrics-hostmetricsreceiver.otel-default
            | STATS cpu = avg(avg_over_time(`metrics.system.cpu.load_average.1m`)) BY host.name, BUCKET(@timestamp, 5 minute)"
}
```

```
"took": 3475,
"is_partial": false,
"documents_found": 11368089,
"values_loaded": 34248167
```

```
"took": 1965,
"is_partial": false,
"documents_found": 11368089,
"values_loaded": 34248167
```
@dnhatn dnhatn force-pushed the backport/8.19/pr-127949 branch from 2015e37 to 34e45a9 Compare May 20, 2025 05:41
@elasticsearchmachine elasticsearchmachine merged commit fd4fc60 into elastic:8.19 May 20, 2025
15 checks passed
@dnhatn dnhatn deleted the backport/8.19/pr-127949 branch May 20, 2025 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants