Skip to content

Emit the script field in ExtendedStatsAggregationBuilder (re-applies #2700) - #4100

Merged
Philippus merged 1 commit into
Philippus:series/9.xfrom
fupelaqu:fix/extended-stats-script-emission
Sep 6, 2026
Merged

Emit the script field in ExtendedStatsAggregationBuilder (re-applies #2700)#4100
Philippus merged 1 commit into
Philippus:series/9.xfrom
fupelaqu:fix/extended-stats-script-emission

Conversation

@fupelaqu

@fupelaqu fupelaqu commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What

ExtendedStatsAggregationBuilder never serialises agg.script, although ExtendedStatsAggregation carries the field and the DSL accepts it. This adds the same emission every sibling metric builder already has, mirroring MaxAggregationBuilder's idiom:

agg.script.foreach { script =>
  builder.rawField("script", handlers.script.ScriptBuilderFn(script))
}

Why it matters

The failure is silent: with both a field and a script set, the emitted JSON contains only "field", so Elasticsearch computes the extended stats over the raw field and returns a plausible, wrong number — no error anywhere. Checked against the current sources: ExtendedStats is the only one of the ten metric aggregation builders with this omission (Avg, Cardinality, Max, Min, Percentiles, Stats, Sum, ValueCount and WeightedAvg all emit script).

History — this fix was already approved once

This is a re-application of #2700 (approved and merged on 2022-08-25), which landed on release/7.10.x and was never forward-ported — so the omission is still present in 7.17.25, 8.18.x and series/9.x. The patch here is #2700 adapted to the current handlers.script.ScriptBuilderFn location and the customAggregations signature.

Test

The existing SearchDslTest case "should generate correct json for extendedstats aggregation" already sets Script("doc['grade'].value").lang("lua") on the aggregation — its golden fixture matched the dropped-script output, i.e. the test was pinning the bug. The fixture is updated to expect the script node (same fixture change as #2700).

sbt "tests/testOnly com.sksamuel.elastic4s.search.SearchDslTest -- -z extendedstats"

green locally on JDK 17 / sbt 2.0.8 — and verified red without the builder change (the fixture update alone fails against the unpatched builder), so the test genuinely asserts the emission.

Backport ask

Would you consider a backport to the 7.17 line (a 7.17.26)? We drive ES 6.x–9.x from one SQL engine (SOFTNETWORK-APP/SoftClient4ES#222) on nl.gn0s1s 8.18.x and 7.17.25, and on 7.x our only current option is to reject scripted extended_stats loudly rather than let it answer wrongly. Happy to open the backport PR against the 7.17 branch if you point me at it.

ExtendedStatsAggregation carries a script and the DSL accepts one, but the
builder never serialised it — the only metric aggregation builder with the
omission (Avg, Cardinality, Max, Min, Percentiles, Stats, Sum, ValueCount and
WeightedAvg all emit it). A scripted extended_stats therefore silently
computed over the raw field when a field was also set.

Same change as Philippus#2700, which was approved and merged in 2022 — into
release/7.10.x, and never forward-ported. The existing SearchDslTest case
already sets a script on the aggregation; its golden fixture matched the
dropped-script output and is updated accordingly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Philippus

Copy link
Copy Markdown
Owner

Nice catch! If you could make the same change towards the 7.x branch series/7.x and the 8.x branch series/8.x that would be great. If not, I can take care of it.

Philippus pushed a commit that referenced this pull request Sep 6, 2026
Backport of #4100 to series/7.x, as requested by the maintainer.
ExtendedStatsAggregation carries a script and the DSL accepts one, but the
builder never serialised it — the only metric aggregation builder with the
omission; the existing SearchDslTest golden fixture pinned the dropped-script
output and is updated accordingly.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Philippus
Philippus merged commit b020a19 into Philippus:series/9.x Sep 6, 2026
3 checks passed
Philippus pushed a commit that referenced this pull request Sep 6, 2026
Backport of #4100 to series/8.x, as requested by the maintainer.
ExtendedStatsAggregation carries a script and the DSL accepts one, but the
builder never serialised it — the only metric aggregation builder with the
omission; the existing SearchDslTest golden fixture pinned the dropped-script
output and is updated accordingly.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Philippus

Copy link
Copy Markdown
Owner

@fupelaqu elastic4s 7.17.26 should be released, please try it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants