Skip to content

Adds better challenges for comparing inlinestats#873

Merged
ncordon merged 4 commits intoelastic:masterfrom
ncordon:better-inlinestats-comparison
Oct 28, 2025
Merged

Adds better challenges for comparing inlinestats#873
ncordon merged 4 commits intoelastic:masterfrom
ncordon:better-inlinestats-comparison

Conversation

@ncordon
Copy link
Contributor

@ncordon ncordon commented Oct 2, 2025

We were using inlinestats queries with the limit after the index querying:

FROM nyc_taxis |  LIMIT 1000 | inlinestats s1 = sum(passenger_count) | inlinestats s2 = sum(s1) | inlinestats s3 = sum(s2)

and we were observing weird results in the benchmarks, possibly due to this issue, where a query with just one inline stats would be faster than the one above:

FROM nyc_taxis | inlinestats s1 = sum(passenger_count)
inlinestats

I arrived at the conclusion those are not a good benchmark, because the time used to fetch the rows from disk is dominating the query time.

This PR changes all of those queries to have the limit at the end:

FROM nyc_taxis | inline stats s1 = sum(passenger_count) | inline stats s2 = sum(trip_distance) | inline stats s3 = sum(total_amount) | LIMIT 1000

This PR also changes inlinestats by inline stats, given the first one seems deprecated when using the _query api:

#! Line 1:84: INLINESTATS is deprecated, use INLINE STATS instead

@gbanasiak
Copy link
Contributor

@elasticmachine update branch

@gbanasiak gbanasiak requested a review from a team October 6, 2025 17:46
Copy link
Member

@gareth-ellis gareth-ellis left a comment

Choose a reason for hiding this comment

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

LGTM - remember that once this is merged the nightly benchmark charts will need updating (there seems to be inline related graphs there

Copy link

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@ncordon ncordon merged commit c41a950 into elastic:master Oct 28, 2025
28 checks passed
@ncordon ncordon deleted the better-inlinestats-comparison branch October 28, 2025 14:00
@esbenchmachine esbenchmachine added the backport pending Awaiting backport to stable release branch label Oct 28, 2025
@gbanasiak
Copy link
Contributor

backported to 9.2 in #919

@gbanasiak gbanasiak added v9.2 and removed backport pending Awaiting backport to stable release branch labels Nov 20, 2025
@elastic elastic deleted a comment from esbenchmachine Nov 21, 2025
@esbenchmachine esbenchmachine mentioned this pull request Dec 3, 2025
gbanasiak added a commit that referenced this pull request Dec 3, 2025
This is an empty commit which records missing backports from manual
or squashed backports through "cherry picked from" metadata.

CI determines Elasticsearch build arguments #925 (#926)
(cherry picked from commit 8c33ff5)

Exclude some challenges when testing with ES release builds #922 (#919)
(cherry picked from commit f38f8fc)

Reduce filtering scope in CI workflow #908 (#919)
(cherry picked from commit 8e571a5)

Address pytest deprecations #911 (#919)
(cherry picked from commit fa81c5e)

Solves problems with expensive inlinestats benchmarks #896 (#919)
(cherry picked from commit 8209244)

Adds better challenges for comparing inlinestats #873 (#919)
(cherry picked from commit c41a950)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants