ESQL: revive inlinestats #122257
ESQL: revive inlinestats #122257
Conversation
elasticsearchmachine
added
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @astefan, I've created a changelog YAML for you. |
| @@ -2,7 +2,7 @@ | |||
| // TODO: re-enable the commented tests once the Join functionality stabilizes | |||
| // | |||
|
|
|||
| maxOfInt-Ignore | |||
| maxOfInt | |||
| required_capability: join_planning_v1 | |||
There was a problem hiding this comment.
Probably we'll need a new capability to avoid running this with old nodes on bwc tests.
| @@ -330,7 +330,7 @@ required_capability: join_planning_v1 | |||
|
|
|||
| FROM airports | |||
| | RENAME scalerank AS int | |||
| | LOOKUP int_number_names ON int | |||
| | LOOKUP JOIN int_number_names ON int | |||
There was a problem hiding this comment.
I think this is incorrect, this test is still disabled and used to use LOOKUP_ 🐔 in the past. (But we probably also want a version of this with LOOKUP JOIN.)
| if (join instanceof InlineJoin) { | ||
| return new FragmentExec(bp); | ||
| } |
There was a problem hiding this comment.
This is odd. What do we need this for? If this is a quick hack, can we add a TODO comment?
I'd think this would prevent the if (right instanceof LocalSourceExec localData) { branch below from ever triggering, which used to be the branch that mapped a physical operation to INLINESTATS.
It seems like this, instead, pushes the execution of the inline join into the data nodes, which cannot always be true - e.g. there could be a regular STATS before the INLINESTATS, which requries execution on the data node. Or just a LIMIT/SORT, or any other pipeline breaker.
There was a problem hiding this comment.
After the inlinestats has been rewritten to allow lookup join to work in a more correct way, what was left for inlinestats was an incomplete solution (inlinestats.csv-spec was ignored in its entirety). EsqlSession code flow expects a fragment for inlinestats that contains logical plans for its right and left branches. It's planning then the right branch, executes it, then it comes back to its left branch and uses the right results as constant blocks in the left logical plan tree.
There are still many queries muted in the inlinestats.csv-spec test and I don't want to go the rabbit hole and try to fix everything in one go. I want on purpose to limit the extent of this PR and I will look further into fixing the rest in followups. If the followups lead to changing the fragment approach, that's ok.
…ticsearch into inlinestats_pickup1
…inlinestats_pickup1
| * Fixes a series of issues with inlinestats which had an incomplete implementation after lookup and inlinestats | ||
| * were refactored. | ||
| */ | ||
| INLINESTATS_STUBRELATION_PROPER_REPLACEMENT; |
There was a problem hiding this comment.
Maybe pick a smaller name that we can version while iterating on the implementation, for example: INLINESTATS_V2 ?
…inlinestats_pickup1
(cherry picked from commit 4b3acd4) Don't run the csv tests for inlinestats in non-snapshot tests (elastic#122407) (cherry picked from commit f8fb3c3)
First take at enabling inlinestats tests.