ESQL: Fix AsyncOperator status values and add emitted rows by ivancea · Pull Request #132738 · elastic/elasticsearch

ivancea · 2025-08-12T15:10:52Z

Some fixes around AsyncOperator.Status:

Fix its pages received/emitted, that were -1 of what they should be (And removed the "Max(0, ...)" to avoid falsifying it
Added the emitted_rows field to LookupFromIndexOperator.Status, which was missing, and would add interesting data on join results
Added better testing support for Status checks after a Driver run

ivancea · 2025-08-13T13:39:59Z

...ugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/AnyOperatorTestCase.java

+                assertThat(map, hasEntry(is("received_pages"), nonNegativeMatcher));
+                assertThat(map, hasEntry(is("completed_pages"), nonNegativeMatcher));
+                assertThat(map, hasEntry(is("process_nanos"), nonNegativeMatcher));


The async operator status fields are serialized as "received_pages" instead of "pages_received" (Like the others operators).
Should I normalize it here? It's not even a transport version change, just some unit tests

++ to make this consistent with the other operators.

elasticsearchmachine · 2025-08-13T13:45:58Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2025-08-13T13:45:58Z

Hi @ivancea, I've created a changelog YAML for you.

dnhatn

@ivancea Thanks for fixing this. LGTM!

dnhatn · 2025-08-13T15:19:35Z

...ugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/AnyOperatorTestCase.java

+                assertThat(map, hasEntry(is("received_pages"), nonNegativeMatcher));
+                assertThat(map, hasEntry(is("completed_pages"), nonNegativeMatcher));
+                assertThat(map, hasEntry(is("process_nanos"), nonNegativeMatcher));


++ to make this consistent with the other operators.

dnhatn · 2025-08-13T15:21:03Z

x-pack/plugin/esql/compute/src/main/java/org/elasticsearch/compute/operator/AsyncOperator.java

    @Override
    public final Operator.Status status() {
-        return status(Math.max(0L, checkpoint.getMaxSeqNo()), Math.max(0L, checkpoint.getProcessedCheckpoint()), processNanos.sum());
+        return status(checkpoint.getMaxSeqNo() + 1, checkpoint.getProcessedCheckpoint() + 1, processNanos.sum());


👍 These are correct usages.

) Fixes #128030 Fixes #130296 Fixes #130642 Fixes #131148 Fixes #132554 Fixes #132555 Fixes #132604 Fixes #131563 Fixes #132778 Extracted from #132738 An AsyncOperator listener misordering caused the warnings collection and status metrics updates to be executed after the `onSeqNoCompleted()`>`notifyIfBlocked()`>`future.onResponse(null)`, which ends the processing in some cases.

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperator.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperatorStatusTests.java

…tic#132744) Fixes elastic#128030 Fixes elastic#130296 Fixes elastic#130642 Fixes elastic#131148 Fixes elastic#132554 Fixes elastic#132555 Fixes elastic#132604 Fixes elastic#131563 Fixes elastic#132778 Extracted from elastic#132738 An AsyncOperator listener misordering caused the warnings collection and status metrics updates to be executed after the `onSeqNoCompleted()`>`notifyIfBlocked()`>`future.onResponse(null)`, which ends the processing in some cases.

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/AnyOperatorTestCase.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexOperatorTests.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/inference/InferenceOperatorTestCase.java

ivancea · 2025-08-20T11:19:46Z

...ugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/AnyOperatorTestCase.java

+    /**
+     * EqualTo matcher that takes care of whole number types (Integers and longs).
+     */
+    protected final Matcher<Object> matchNumberEqualTo(Number value) {
+        return wholeMatcher(comparesEqualTo(value.intValue()), comparesEqualTo(value.longValue()));
+    }
+
+    /**
+     * GreaterThanOrEqualTo matcher that takes care of whole number types (Integers and longs).
+     */
+    protected final Matcher<Object> matchNumberGreaterThanOrEqualTo(Number value) {
+        return wholeMatcher(greaterThanOrEqualTo(value.intValue()), greaterThanOrEqualTo(value.longValue()));
+    }
+
+    /**
+     * Matcher that matches based on the number type (Integer or long).
+     */
+    @SuppressWarnings("unchecked")
+    protected final Matcher<Object> wholeMatcher(Matcher<Integer> integerMatcher, Matcher<Long> longMatcher) {
+        return either(both(instanceOf(Integer.class)).and((Matcher<? super Object>) (Matcher<?>) integerMatcher)).or(
+            both(instanceOf(Long.class)).and((Matcher<? super Object>) (Matcher<?>) longMatcher)
+        );
+    }


Without those, we can't safely make a "greater than 0" matcher, for example, as it could be either an int or a long. This takes care of that. I couldn't find a better option.

either(greaterThan(0)).or(greaterThan(0L))?

I found that they always made 0 the Integer when deserializing unless the result is huge.

Changed! It required some not-technically-correct casts, but worked fine

nik9000 · 2025-08-20T14:19:02Z

...ugin/esql/compute/test/src/main/java/org/elasticsearch/compute/test/AnyOperatorTestCase.java

+    /**
+     * EqualTo matcher that takes care of whole number types (Integers and longs).
+     */
+    protected final Matcher<Object> matchNumberEqualTo(Number value) {
+        return wholeMatcher(comparesEqualTo(value.intValue()), comparesEqualTo(value.longValue()));
+    }
+
+    /**
+     * GreaterThanOrEqualTo matcher that takes care of whole number types (Integers and longs).
+     */
+    protected final Matcher<Object> matchNumberGreaterThanOrEqualTo(Number value) {
+        return wholeMatcher(greaterThanOrEqualTo(value.intValue()), greaterThanOrEqualTo(value.longValue()));
+    }
+
+    /**
+     * Matcher that matches based on the number type (Integer or long).
+     */
+    @SuppressWarnings("unchecked")
+    protected final Matcher<Object> wholeMatcher(Matcher<Integer> integerMatcher, Matcher<Long> longMatcher) {
+        return either(both(instanceOf(Integer.class)).and((Matcher<? super Object>) (Matcher<?>) integerMatcher)).or(
+            both(instanceOf(Long.class)).and((Matcher<? super Object>) (Matcher<?>) longMatcher)
+        );
+    }


either(greaterThan(0)).or(greaterThan(0L))?

I found that they always made 0 the Integer when deserializing unless the result is huge.

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

ivancea added 5 commits August 12, 2025 15:49

Fixed status variables naming

283c2d5

Fixed notifying threads before updating stats and warnings

010d915

Fixed status values for AsyncOperator

262f488

Added emittedRows and transport version

35d2ca2

Added capability for the fix

916f591

ivancea added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v9.2.0 labels Aug 12, 2025

Unmuted tests

5d74c2d

ivancea mentioned this pull request Aug 12, 2025

ESQL: Fix async operator warnings not always sent when blocking #132744

Merged

ivancea changed the title ~~ESQL: Fix Driver finishing before AsyncOperator listener~~ ESQL: (WIP) Fix Driver finishing before AsyncOperator listener Aug 12, 2025

ivancea added 2 commits August 13, 2025 12:49

Undo extracted PR changes

007fd4a

Merge branch 'main' into esql-lookup-warnings-status-race

0da2e4e

ivancea changed the title ~~ESQL: (WIP) Fix Driver finishing before AsyncOperator listener~~ ESQL: Fix AsyncOperator status values and add emitted rows Aug 13, 2025

ivancea added 2 commits August 13, 2025 15:30

Remove unused supress warnings

55f8804

Remove TODO

5535503

ivancea commented Aug 13, 2025

View reviewed changes

Merge branch 'main' into esql-lookup-warnings-status-race

ba84ddd

ivancea added >bug and removed >non-issue labels Aug 13, 2025

ivancea marked this pull request as ready for review August 13, 2025 13:45

Update docs/changelog/132738.yaml

4afdc80

dnhatn self-requested a review August 13, 2025 14:40

dnhatn approved these changes Aug 13, 2025

View reviewed changes

Rename received_pages to pages_received, and similar status fields

121ce8a

Update test after merge

5dbbd22

ivancea and others added 6 commits August 19, 2025 15:27

Improve Lookup operator test for status

bf4aea8

[CI] Auto commit changes from spotless

e2de5ed

Fix assertion type for map numbers

6a505b0

Fix Lucene tests operator status check

25ef602

Update Limit operator tests

e8e9226

ivancea added the test-release Trigger CI checks against release build label Aug 20, 2025

Merge branch 'main' into esql-lookup-warnings-status-race

9c64a0b

ivancea commented Aug 20, 2025

View reviewed changes

nik9000 approved these changes Aug 20, 2025

View reviewed changes

ivancea added 2 commits August 21, 2025 12:06

Merge branch 'main' into esql-lookup-warnings-status-race

523d542

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

Remove custom matching functions

b56abca

ivancea enabled auto-merge (squash) August 21, 2025 10:42

Merge branch 'main' into esql-lookup-warnings-status-race

675d171

ivancea merged commit 6066ac4 into elastic:main Aug 21, 2025
34 checks passed

ivancea deleted the esql-lookup-warnings-status-race branch August 21, 2025 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

ESQL: Fix AsyncOperator status values and add emitted rows#132738

ESQL: Fix AsyncOperator status values and add emitted rows#132738
ivancea merged 25 commits intoelastic:mainfrom
ivancea:esql-lookup-warnings-status-race

ivancea commented Aug 12, 2025 •

edited

Loading

Uh oh!

ivancea Aug 13, 2025 •

edited

Loading

Uh oh!

dnhatn Aug 13, 2025

Uh oh!

elasticsearchmachine commented Aug 13, 2025

Uh oh!

elasticsearchmachine commented Aug 13, 2025

Uh oh!

dnhatn left a comment

Uh oh!

dnhatn Aug 13, 2025

Uh oh!

dnhatn Aug 13, 2025

Uh oh!

ivancea Aug 20, 2025

Uh oh!

nik9000 Aug 20, 2025

Uh oh!

ivancea Aug 21, 2025

Uh oh!

nik9000 Aug 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

Conversation

ivancea commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivancea Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Aug 13, 2025

Uh oh!

elasticsearchmachine commented Aug 13, 2025

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

dnhatn Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

dnhatn Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

ivancea Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

nik9000 Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

ivancea Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

nik9000 Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivancea commented Aug 12, 2025 •

edited

Loading

ivancea Aug 13, 2025 •

edited

Loading