Skip to content

ESQL: Added Sample operator NamedWritable to plugin#131541

Merged
ivancea merged 5 commits intoelastic:mainfrom
ivancea:fix-sample-operator-serialization
Jul 21, 2025
Merged

ESQL: Added Sample operator NamedWritable to plugin#131541
ivancea merged 5 commits intoelastic:mainfrom
ivancea:fix-sample-operator-serialization

Conversation

@ivancea
Copy link
Contributor

@ivancea ivancea commented Jul 18, 2025 •
edited
Loading

SampleOperator.Status wasn't declared as a NamedWritable by the plugin, leading to serialization errors when SAMPLE is used with profile: true.

It leads to an IllegalArgumentException: Unknown NamedWriteable [org.elasticsearch.compute.operator.Operator$Status][sample]

Profiles will be tested in this PR: #131474, that's currently failing because of this bug
@ivancea ivancea requested a review from jan-elastic July 18, 2025 15:33
@ivancea ivancea added >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.2.0 v9.1.1 labels Jul 18, 2025
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 18, 2025

Hi @ivancea, I've created a changelog YAML for you.
nik9000
nik9000 reviewed 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.

Adding a TransportVersion and a minimumSupportedVersion should make it skipped for versions that don't support it.

I would recommend making a 9.2 and 9.1 one.
jan-elastic
jan-elastic approved these changes Jul 21, 2025
View reviewed changes
Copy link
Contributor

@jan-elastic jan-elastic left a comment

Choose a reason for hiding this comment

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

LGTM
@ivancea ivancea force-pushed the fix-sample-operator-serialization branch from cdf06ed to b321464 Compare July 21, 2025 10:44
@ivancea ivancea marked this pull request as ready for review July 21, 2025 10:50
@ivancea ivancea requested review from jan-elastic and nik9000 July 21, 2025 10:50
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 21, 2025

Pinging @elastic/es-analytical-engine (Team:Analytics)
jan-elastic
jan-elastic reviewed Jul 21, 2025
View reviewed changes
x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/SampleOperator.java
int sampledIdx = 0;
for (int i = randomSamplingIterator.docID(); i - rowsReceived < page.getPositionCount(); i = randomSamplingIterator.nextDoc()) {
sampledPositions[sampledIdx++] = i - rowsReceived;
sampledPositions[sampledIdx++] = Math.toIntExact(i - rowsReceived);
Copy link
Contributor

@jan-elastic jan-elastic Jul 21, 2025

Choose a reason for hiding this comment

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

RandomSamplingIterator uses an int for maxDoc. So if more than MAX_INT rows are process, something else breaks.
Copy link
Contributor

@jan-elastic jan-elastic Jul 21, 2025

Choose a reason for hiding this comment

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

Not really an issue introduced by this PR though
jan-elastic
jan-elastic approved these changes Jul 21, 2025
View reviewed changes
Copy link
Contributor

@jan-elastic jan-elastic left a comment

Choose a reason for hiding this comment

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

LGTM
@ivancea ivancea merged commit aa77c4a into elastic:main Jul 21, 2025
33 checks passed
@ivancea ivancea deleted the fix-sample-operator-serialization branch July 21, 2025 12:27
ivancea added a commit to ivancea/elasticsearch that referenced this pull request Jul 21, 2025
@ivancea
ESQL: Added Sample operator NamedWritable to plugin (elastic#131541)
d4dd907 
`SampleOperator.Status` wasn't declared as a NamedWritable by the plugin, leading to serialization errors when `SAMPLE` is used with `profile: true`.

It leads to an `IllegalArgumentException: Unknown NamedWriteable [org.elasticsearch.compute.operator.Operator$Status][sample]`

Profiles will be tested in this PR: elastic#131474, that's currently failing because of this bug
@ivancea ivancea mentioned this pull request Jul 21, 2025
Merged
ivancea added a commit that referenced this pull request Jul 21, 2025
@ivancea
[9.1] ESQL: Added Sample operator NamedWritable to plugin (#131541) (#…
9d82c48 
…131621)

Manual 9.1 backport of #131541

There's a TransportVersion change here
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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

5 participants

@ivancea @elasticsearchmachine @nik9000 @idegtiarenko @jan-elastic