ESQL: Don't duplicate Limit on left InlineJoin #133651
Conversation
...as this reduces the data the (inline) aggs run on, which is wrong.
bpintea
requested review from
alex-spies and
astefan
and removed request for
astefan
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
elasticsearchmachine
added
the
Team:Analytics
| } | ||
| } | ||
| } else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT) { | ||
| } else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT && join instanceof InlineJoin == false) { |
There was a problem hiding this comment.
++.
Maybe let's add a comment that we want to also apply this to InlineJoin, but that requires that the stub relation corresponds to a node somewhere inside the left branch, rather than the whole left branch. Or we need to get rid of the stub relation then and replace it by a copy of the left branch (with updated name ids, most likely, but that's a detail).
| FROM employees | ||
| | KEEP emp_no, languages, gender | ||
| | INLINESTATS max_lang = MAX(languages) BY gender | ||
| | LIMIT 1 |
There was a problem hiding this comment.
Limit 1 is non-deterministic and this might fail on multi-node tests like for serverless, no?
Maybe LIMIT 3 | SORT gender (-> F, M, null) as this would still surface the bug?
There was a problem hiding this comment.
The first (emp_no-sorted) value has a language value of 2. If the LIMIT (of 1) is pushed down, that would yield a max_lang of 2. Ideally we'd have a | SORT emp_no before INLINESTATS, but that doesn't current work.
In any case, even if the data is shuffled, all records should have the same max_lang (5).
I've left a comment on the test.
| } | ||
| } | ||
| } else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT) { | ||
| } else if (limit.child() instanceof Join join && join.config().type() == JoinTypes.LEFT && join instanceof InlineJoin == false) { |
There was a problem hiding this comment.
A one line comment here would help, imho.
|
Thanks Alex, Andrei. |
bpintea
added
the
auto-merge-without-approval
...as this reduces the data the (inline) aggs run on, which is wrong. Closes elastic#133235
4 participants
...as this reduces the data the (inline) aggs run on, which is wrong.
Closes #133235