Skip to content

Disk indicator review#7

Merged
masseyke merged 2 commits intomasseyke:feature/disk-health-indicatorfrom
gmarouli:disk-indicator-review
Sep 19, 2022
Merged

Disk indicator review#7
masseyke merged 2 commits intomasseyke:feature/disk-health-indicatorfrom
gmarouli:disk-indicator-review

Conversation

@gmarouli
Copy link
Copy Markdown

@gmarouli gmarouli commented Sep 19, 2022

As requested, I am applying the review requests in a PR to speed up the process. Here I am listing them with an explanation in regards to why I thought this was a good idea.

  • Use only one cluster state in a single calculation. I think this will help with the stability of the calculation and the debugging if issues arise. Furthermore, it makes explicit that in order to de-reference the roles and names we need the cluster state.

@masseyke masseyke marked this pull request as ready for review September 19, 2022 17:27
@masseyke masseyke merged commit b31fa7e into masseyke:feature/disk-health-indicator Sep 19, 2022
@gmarouli gmarouli deleted the disk-indicator-review branch August 20, 2024 06:56
masseyke pushed a commit that referenced this pull request Oct 7, 2024
masseyke pushed a commit that referenced this pull request Jan 23, 2026
…tic#140027)

This PR fixes the issue where `INLINE STATS GROUP BY null` was being
incorrectly pruned by `PruneLeftJoinOnNullMatchingField`.

Fixes elastic#139887

## Problem For query:

```
FROM employees
| INLINE STATS c = COUNT(*) BY n = null
| KEEP c, n
| LIMIT 3
```

During `LogicalPlanOptimizer`:

```
Limit[3[INTEGER],false,false]
\_EsqlProject[[c{r}#2, n{r}#4]]
  \_InlineJoin[LEFT,[n{r}#4],[n{r}#4]]
    |_Eval[[null[NULL] AS n#4]]
    | \_EsRelation[employees][<no-fields>{r$}#7]
    \_Aggregate[[n{r}#4],[COUNT(*[KEYWORD],true[BOOLEAN],PT0S[TIME_DURATION]) AS c#2, n{r}#4]]
      \_StubRelation[[<no-fields>{r$}#7, n{r}#4]]
```

The following join node:

```
InlineJoin[LEFT,[n{r}#4],[n{r}#4]]
|_Eval[[null[NULL] AS n#4]]
| \_EsRelation[employees][<no-fields>{r$}#7]
\_Aggregate[[n{r}#4],[COUNT(*[KEYWORD],true[BOOLEAN],PT0S[TIME_DURATION]) AS c#2, n{r}#4]]
  \_StubRelation[[<no-fields>{r$}#7, n{r}#4]]
```

should NOT have `PruneLeftJoinOnNullMatchingField` applied, because the
right side is an `Aggregate` (originating from `INLINE STATS`). Since
`STATS` supports `GROUP BY null`, the join key being null is a valid use
case. Pruning this join would incorrectly eliminate the aggregation
results, changing the query semantics.

During `LocalLogicalPlanOptimizer`:

```
ProjectExec[[c{r}#2, n{r}#4]]
\_LimitExec[3[INTEGER],null]
  \_ExchangeExec[[c{r}#2, n{r}#4],false]
    \_FragmentExec[filter=null, estimatedRowSize=0, reducer=[], fragment=[<>
Project[[c{r}#2, n{r}#4]]
\_Limit[3[INTEGER],false,false]
  \_InlineJoin[LEFT,[n{r}#4],[n{r}#4]]
    |_Eval[[null[NULL] AS n#4]]
    | \_EsRelation[employees][<no-fields>{r$}#7]
    \_LocalRelation[[c{r}#2, n{r}#4],Page{blocks=[LongVectorBlock[vector=ConstantLongVector[positions=1, value=100]], ConstantNullBlock[positions=1]]}]<>]]
```

The following join node:

```
InlineJoin[LEFT,[n{r}#4],[n{r}#4]]
|_Eval[[null[NULL] AS n#4]]
| \_EsRelation[employees][<no-fields>{r$}#7]
\_LocalRelation[[c{r}#2, n{r}#4],Page{blocks=[LongVectorBlock[vector=ConstantLongVector[positions=1, value=100]], ConstantNullBlock[positions=1]]}]
```

should NOT have `PruneLeftJoinOnNullMatchingField` applied, because the
right side is a `LocalRelation` (the `Aggregate` was optimized into a
`LocalRelation` containing the pre-computed aggregation results).
Pruning this join when the join key is null would discard the valid
aggregation results stored in the `LocalRelation`, incorrectly producing
null values instead of the expected count.

## Solution The fix ensures that `PruneLeftJoinOnNullMatchingField` only
applies to `LOOKUP JOIN` nodes, where `join.right()` is an `EsRelation`.
For `INLINE STATS` joins, the right side can be:

 - `Aggregate` (before optimization), or
 - `LocalRelation` (after the aggregate is optimized)

By checking `join.right() instanceof EsRelation`, we correctly skip the
pruning optimization for `INLINE STATS` joins, preserving the expected
query results when grouping by null.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants