Skip to content

Add optimized path for intermediate values aggregator#131390

Merged
dnhatn merged 10 commits intoelastic:mainfrom
dnhatn:values-partial-input
Jul 21, 2025
Merged

Add optimized path for intermediate values aggregator#131390
dnhatn merged 10 commits intoelastic:mainfrom
dnhatn:values-partial-input

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Jul 16, 2025 •
edited
Loading

Similar to #127849, this change adds an optimized path for leveraging ordinal blocks of intermediate input pages in the Values aggregator. Below are the micro-benchmark results.

Before:

// 1 raw input page + 1000 intermediate input pages
Benchmark                      (dataType)  (groups)  Mode  Cnt       Score   Error  Units
ValuesAggregatorBenchmark.run    BytesRef         1  avgt    2       0.382          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000  avgt    2     112.293          ms/op
ValuesAggregatorBenchmark.run    BytesRef   1000000  avgt    2  113182.908          ms/op

After:
// 1 raw input page + 1000 intermediate input pages
Benchmark                      (dataType)  (groups)  Mode  Cnt      Score   Error  Units
ValuesAggregatorBenchmark.run    BytesRef         1  avgt    2      0.378          ms/op
ValuesAggregatorBenchmark.run    BytesRef      1000  avgt    2     34.410          ms/op
ValuesAggregatorBenchmark.run    BytesRef   1000000  avgt    2  64654.830          ms/op

1K groups: 112 ms -> 34.4ms
1M groups: 113s -> 64s

More to come with #130510

Relates #127849
@dnhatn dnhatn force-pushed the values-partial-input branch from d336a28 to f854f10 Compare July 17, 2025 00:43
@dnhatn dnhatn changed the title Add prepareProcessIntermediateInputPage Jul 17, 2025
@dnhatn dnhatn force-pushed the values-partial-input branch from f854f10 to 42b033f Compare July 17, 2025 03:14
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 17, 2025

Hi @dnhatn, I've created a changelog YAML for you.
@dnhatn dnhatn force-pushed the values-partial-input branch from 3f51173 to 747d56e Compare July 17, 2025 05:03
@dnhatn dnhatn requested review from ivancea and nik9000 July 17, 2025 05:33
@dnhatn dnhatn marked this pull request as ready for review July 17, 2025 05:33
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 17, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 17, 2025

Pinging @elastic/es-analytical-engine (Team:Analytics)
@dnhatn dnhatn requested a review from idegtiarenko July 18, 2025 06:46
nik9000
nik9000 approved these changes Jul 18, 2025
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I just scanned it, but I approve of the general approach of letting aggs optimize their intermediate join and think VALUES is the right place to do it. I'm not sure if you did it right, but I think @idegtiarenko is checking this more closely.
ivancea
ivancea approved these changes Jul 21, 2025
View reviewed changes
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
...l/compute/src/main/java/org/elasticsearch/compute/aggregation/ValuesBytesRefAggregators.java
ordinals = asOrdinals.getOrdinalsBlock();
}
}
if (dict != null && dict.getPositionCount() < groupIds.getPositionCount()) {
Copy link
Contributor

@ivancea ivancea Jul 21, 2025

Choose a reason for hiding this comment

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

Should this use OrdinalBytesRefBlock.isDense(), or are the logics not related?
@dnhatn
Copy link
Member Author

dnhatn commented Jul 21, 2025

Thanks friends!
@dnhatn dnhatn merged commit 2564379 into elastic:main Jul 21, 2025
33 checks passed
@dnhatn dnhatn deleted the values-partial-input branch July 21, 2025 19:14
szybia added a commit to szybia/elasticsearch that referenced this pull request Jul 22, 2025
@szybia
Merge remote-tracking branch 'upstream/main' into index-pipeline-trac…
190db87 
…king

* upstream/main: (100 commits)
  Term vector API on stateless search nodes (elastic#129902)
  TEST Fix ThreadPoolMergeSchedulerStressTestIT testMergingFallsBehindAndThenCatchesUp (elastic#131636)
  Add inference.put_custom rest-api-spec (elastic#131660)
  ESQL: Fewer serverless docs in tests (elastic#131651)
  Skip search on indices with INDEX_REFRESH_BLOCK (elastic#129132)
  Mute org.elasticsearch.indices.cluster.RemoteSearchForceConnectTimeoutIT testTimeoutSetting elastic#131656
  [jdk] Resolve EA OpenJDK builds to our JDK archive (elastic#131237)
  Add optimized path for intermediate values aggregator (elastic#131390)
  Correctly handling download_database_on_pipeline_creation within a pipeline processor within a default or final pipeline (elastic#131236)
  Refresh potential lost connections at query start for `_search` (elastic#130463)
  Add template_id to patterned-text type (elastic#131401)
  Integrate LIKE/RLIKE LIST with ReplaceStringCasingWithInsensitiveRegexMatch rule (elastic#131531)
  [ES|QL] Add doc for the COMPLETION command (elastic#131010)
  ESQL: Add times to topn status (elastic#131555)
  ESQL: Add asynchronous pre-optimization step for logical plan (elastic#131440)
  ES|QL: Improve generative tests for FORK [130015] (elastic#131206)
  Update index mapping update privileges (elastic#130894)
  ESQL: Added Sample operator NamedWritable to plugin (elastic#131541)
  update `kibana_system` to grant it access to `.chat-*` system index (elastic#131419)
  Clarify heap size configuration (elastic#131607)
  ...
szybia added a commit to szybia/elasticsearch that referenced this pull request Jul 22, 2025
@szybia
Merge remote-tracking branch 'upstream/main' into component-templates…
b8f21b1 
…-tracking

* upstream/main: (44 commits)
  Term vector API on stateless search nodes (elastic#129902)
  TEST Fix ThreadPoolMergeSchedulerStressTestIT testMergingFallsBehindAndThenCatchesUp (elastic#131636)
  Add inference.put_custom rest-api-spec (elastic#131660)
  ESQL: Fewer serverless docs in tests (elastic#131651)
  Skip search on indices with INDEX_REFRESH_BLOCK (elastic#129132)
  Mute org.elasticsearch.indices.cluster.RemoteSearchForceConnectTimeoutIT testTimeoutSetting elastic#131656
  [jdk] Resolve EA OpenJDK builds to our JDK archive (elastic#131237)
  Add optimized path for intermediate values aggregator (elastic#131390)
  Correctly handling download_database_on_pipeline_creation within a pipeline processor within a default or final pipeline (elastic#131236)
  Refresh potential lost connections at query start for `_search` (elastic#130463)
  Add template_id to patterned-text type (elastic#131401)
  Integrate LIKE/RLIKE LIST with ReplaceStringCasingWithInsensitiveRegexMatch rule (elastic#131531)
  [ES|QL] Add doc for the COMPLETION command (elastic#131010)
  ESQL: Add times to topn status (elastic#131555)
  ESQL: Add asynchronous pre-optimization step for logical plan (elastic#131440)
  ES|QL: Improve generative tests for FORK [130015] (elastic#131206)
  Update index mapping update privileges (elastic#130894)
  ESQL: Added Sample operator NamedWritable to plugin (elastic#131541)
  update `kibana_system` to grant it access to `.chat-*` system index (elastic#131419)
  Clarify heap size configuration (elastic#131607)
  ...
@dnhatn dnhatn mentioned this pull request Jul 28, 2025
Merged
dnhatn added a commit that referenced this pull request Jul 28, 2025
@dnhatn
Fix off by one in ValuesBytesRefAggregator (#132032)
b720ded 
There are two bugs introduced in #130510 and #131390 affecting the 
VALUES aggregator. The random tests do not cover these edge cases:

1. The check should be firstValues.size() <= group instead of
firstValues.size() < group when reading values from the firstValues
array. We need to inject nulls with repeated values (to simulate
ordinals) to trigger this case.

2. We incorrectly added positionOffset when reading the group ID. We need
to generate more groups to trigger chunking.

Relates #130510 
Relates #131390

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

Labels

:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

5 participants

@dnhatn @elasticsearchmachine @nik9000 @idegtiarenko @ivancea