-
Notifications
You must be signed in to change notification settings - Fork 25.2k
ES|QL: Support STATS after FORK #128745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
ES|QL: Support STATS after FORK #128745
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Hi @ioanatia, I've created a changelog YAML for you. |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
I am putting back the non-issue label, because we don't want a release note for this particular PR. |
if (aggregatorMode == AggregatorMode.INITIAL && aggregateExec.child() instanceof ExchangeSourceExec) { | ||
if (aggregatorMode == AggregatorMode.INITIAL | ||
&& aggregateExec.child() instanceof ExchangeSourceExec exchangeSourceExec | ||
&& exchangeSourceExec.isIntermediateAgg()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks fine to me, but I'd like to get another pair of eyes from @alex-spies to double check if the aggregatorMode
is set properly, as this was introduced by Alex originally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason why we added this check is because a query like:
FROM test
| FORK
( WHERE content:"fox" ) // sub plan 1
( WHERE content:"dog" ) // sub plan 2
| SORT _fork
| KEEP _fork, id, content
will now be split into multiple sub plans using ExchangeSourceExec
and ExchangeSinkExec
:

and so we need to explicitly check with what type of ExchangeSourceExec
we are dealing with.
related: #121950
It was reported that using STATS immediately after FORK results in an error.
Example:
ends up with
The problem has to do with how we set the
aggregatorMode
inAbstractPhysicalOperationProviders.javagroupingPhysicalOperation
. We just assume that if theAggregateExec
follows aExchangeSourceExec
, we should set theaggregatorMode
toINTERMEDIARY
because we are on the data node reduce step.With the introduction of FORK and #126389 this is no longer always true, so we explicitly check if the
ExchangeSourceExec
is indeed between aggregations.