ESQL: Fix flattened fields loading under NULLIFY - #145741
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @GalLalouche, I've created a changelog YAML for you. |
c970365 to
c7700e0
Compare
| \_EsQueryExec[sample_data], indexMode[standard], [_doc{f}#2], limit[1000], sort[] estimatedRowSize[54] queryBuilderAndTags [[QueryBuilderAndTags[query={ | ||
| \_ProjectExec[[message{f}#0, does_not_exist{r}#1]] | ||
| \_FieldExtractExec[message{f}#0]<[],[],[]> | ||
| \_EvalExec[[null[NULL] AS does_not_exist#1]] |
There was a problem hiding this comment.
Here and below: The plan was changed to avoid loading does_not_exist, which makes sense!
alex-spies
left a comment
There was a problem hiding this comment.
Thanks @GalLalouche , the fix makes a lot of sense. This LGTM after we add one or two more unit tests, see my comment below.
Just out of interest, what did you think about these (or other) alternative approaches?
- Insert
nulls in the reduce plan, so that we don't wastefully create null blocks to pass them to the topn operator and later discard most of them. More complex/less isolated? - Amending InsertFieldExtraction to have it insert
EVAL foo = nullin case ofMissingEsField? This goes conceptually againstReplaceFieldWithConstantOrNull, but I often wondered if our approach of inserting a bunch of EVAL expressions on top of EsRelations was the most appropriate to begin with, given that we're overriding field attributes with references attributes with the same name id.
| * Fix for flattened subfields not being nullified when {@code unmapped_fields="nullify"} is set. | ||
| * See https://github.com/elastic/elasticsearch/issues/142616 | ||
| */ | ||
| FIX_NULLIFY_FLATTENED_SUBFIELD, |
There was a problem hiding this comment.
nit: can we use the OPTIONAL_FIELDS and group this with the other nullify/load capabilities?
There was a problem hiding this comment.
Done, and bumped to v6.
There was a problem hiding this comment.
Hm, I'd rather not bump to v6, v7, ... anymore since this is out of snapshot. Depending on the timing of when this gets merged, this may disable valid bwc tests against Serverless. The safer thing is to add new capabilities going forward.
There was a problem hiding this comment.
Ah, got it! Done.
| @@ -146,7 +148,11 @@ | |||
| } | |||
|
|
|||
| private static PhysicalPlan toPhysical(LogicalPlan plan, LocalPhysicalOptimizerContext context) { | |||
There was a problem hiding this comment.
While we're at it, can we add some javadoc to explain this method's purpose?
IIRC, this is a specifically stripped down version of what PlannerUtils#localPlan would do to a data node plan (the thing that's normally in the fragment handed to localPlan).
| var logicalContext = new LocalLogicalOptimizerContext(context.configuration(), context.foldCtx(), context.searchStats()); | ||
| // Replace NULL-typed fields (from UNMAPPED_FIELDS="NULLIFY") with constant nulls so that InsertFieldExtraction doesn't attempt to | ||
| // load them from the index. | ||
| LogicalPlan optimized = new ReplaceFieldWithConstantOrNull().apply(plan, logicalContext); | ||
| return new InsertFieldExtraction().apply(new ReplaceSourceAttributes().apply(LocalMapper.INSTANCE.map(optimized)), context); |
There was a problem hiding this comment.
Can we add a unit/golden test or two that show how this change takes effect on the reduce plan (or, rather, how this leaves the reduce plan intact)?
I think the crux is that without this, we may attempt to late materialize nullified fields, because toPhysical is used to get a simplified data driver plan. And if the simplified data driver plan doesn't tell us that it will actually provide the nullified fields, well, we'll try to field extract them in the reduce driver, right?
It may also make sense to make the comment a bit more explicit about the effect on the reduce plan, specifically.
There was a problem hiding this comment.
It may also make sense to make the comment a bit more explicit about the effect on the reduce plan, specifically.
I'm not sure I understand this ask. This entire method only affects the reduce plan, and the effect is just what the comment says (replace null-typed fields with nulls to avoid trying to load them). I renamed the method to toPhysicalReducePlan to make it clearer, plus the added javadoc as suggested above.
There was a problem hiding this comment.
toPhysicalReducePlan
That's actually not what this method does! It actually takes a data logical plan and turns it into a physical plan with the minimum number of transformations possible while extracting only the necessary fields. That's so we don't have to simulate optimizations, that may be different per shard, to figure out what attributes will already be there after the TopN. A better name may be toNonOptimizedPhysicalPlan or toNonOptimizedPhysicalDataPlan.
So, my comment actually has 2 asks:
- The comment is mechanical. It doesn't give me, as a reader, the context to understand why it's needed. I figured out that it's needed because without
ReplaceFieldWithConstantOrNull, we will treat nullified fields as regular fields and will try to materialize them late. But our mechanism for late materialization relies onInsertFieldExtraction, which actually is the wrong mechanism for nullified fields! Nullified fields want anEVAL nullified = nullto be inserted in the data node plan. So ourtoNonOptimizedPhysicalDataPlanneeds to take this into account. - We need one or two late materialization golden tests with
nullifyto show that the reduce plan does not try to extract nullified fields, even though these fields are not technically used before the topn. In other words, a golden test that would reproduce the problem and fail without this fix.
There was a problem hiding this comment.
You're correct! Thanks for insisting :) I've changed the comment, added golden tests, and created an issue to do this more efficiently in the future: #146068.
|
Also, can we backport this to 9.3 if it's not too much trouble? |
|
Lastly, a PR title nit: this is about flattened subfields, not nested, right? |
740a4cf to
0651d39
Compare
💔 Backport failed
You can use sqren/backport to manually backport by running |
When using `UNMAPPED_FIELDS="NULLIFY"` with flattened field subfields (e.g., `resource.attributes.host.name`), queries hit an `IllegalStateException` (`element_type [BYTES_REF] NOT IN (NULL, NULL)`) or `UnsupportedOperationException` (`can't append non-null values to a null block`). The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with `MissingEsField` / `DataType.NULL`. However, on the data node, `ReplaceFieldWithConstantOrNull` consults `searchStats.exists()` to decide whether to retain the field — and for flattened subfields, `exists()` returns `true` because they are physically mapped in Lucene. This causes the field to be retained and extracted with `ElementType.NULL`, leading to a type mismatch at the compute layer. 1. In `ReplaceFieldWithConstantOrNull`, check for `MissingEsField` before consulting `searchStats.exists()`. If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats. 2. In `LateMaterializationPlanner`, run `ReplaceFieldWithConstantOrNull` in `toPhysical` so that null-typed fields are replaced with constant nulls before `InsertFieldExtraction` runs. This prevents the late materialization path from trying to extract them from the index. Resolves: elastic#142616.
When using `UNMAPPED_FIELDS="NULLIFY"` with flattened field subfields (e.g., `resource.attributes.host.name`), queries hit an `IllegalStateException` (`element_type [BYTES_REF] NOT IN (NULL, NULL)`) or `UnsupportedOperationException` (`can't append non-null values to a null block`). The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with `MissingEsField` / `DataType.NULL`. However, on the data node, `ReplaceFieldWithConstantOrNull` consults `searchStats.exists()` to decide whether to retain the field — and for flattened subfields, `exists()` returns `true` because they are physically mapped in Lucene. This causes the field to be retained and extracted with `ElementType.NULL`, leading to a type mismatch at the compute layer. 1. In `ReplaceFieldWithConstantOrNull`, check for `MissingEsField` before consulting `searchStats.exists()`. If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats. 2. In `LateMaterializationPlanner`, run `ReplaceFieldWithConstantOrNull` in `toPhysical` so that null-typed fields are replaced with constant nulls before `InsertFieldExtraction` runs. This prevents the late materialization path from trying to extract them from the index. Resolves: elastic#142616.
When using `UNMAPPED_FIELDS="NULLIFY"` with flattened field subfields (e.g., `resource.attributes.host.name`), queries hit an `IllegalStateException` (`element_type [BYTES_REF] NOT IN (NULL, NULL)`) or `UnsupportedOperationException` (`can't append non-null values to a null block`). The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with `MissingEsField` / `DataType.NULL`. However, on the data node, `ReplaceFieldWithConstantOrNull` consults `searchStats.exists()` to decide whether to retain the field — and for flattened subfields, `exists()` returns `true` because they are physically mapped in Lucene. This causes the field to be retained and extracted with `ElementType.NULL`, leading to a type mismatch at the compute layer. 1. In `ReplaceFieldWithConstantOrNull`, check for `MissingEsField` before consulting `searchStats.exists()`. If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats. 2. In `LateMaterializationPlanner`, run `ReplaceFieldWithConstantOrNull` in `toPhysical` so that null-typed fields are replaced with constant nulls before `InsertFieldExtraction` runs. This prevents the late materialization path from trying to extract them from the index. Resolves: #142616.
When using `UNMAPPED_FIELDS="NULLIFY"` with flattened field subfields (e.g., `resource.attributes.host.name`), queries hit an `IllegalStateException` (`element_type [BYTES_REF] NOT IN (NULL, NULL)`) or `UnsupportedOperationException` (`can't append non-null values to a null block`). The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with `MissingEsField` / `DataType.NULL`. However, on the data node, `ReplaceFieldWithConstantOrNull` consults `searchStats.exists()` to decide whether to retain the field — and for flattened subfields, `exists()` returns `true` because they are physically mapped in Lucene. This causes the field to be retained and extracted with `ElementType.NULL`, leading to a type mismatch at the compute layer. 1. In `ReplaceFieldWithConstantOrNull`, check for `MissingEsField` before consulting `searchStats.exists()`. If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats. 2. In `LateMaterializationPlanner`, run `ReplaceFieldWithConstantOrNull` in `toPhysical` so that null-typed fields are replaced with constant nulls before `InsertFieldExtraction` runs. This prevents the late materialization path from trying to extract them from the index. Resolves: #142616.
## Issue When using `UNMAPPED_FIELDS="NULLIFY"` with flattened field subfields (e.g., `resource.attributes.host.name`), queries hit an `IllegalStateException` (`element_type [BYTES_REF] NOT IN (NULL, NULL)`) or `UnsupportedOperationException` (`can't append non-null values to a null block`). The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with `MissingEsField` / `DataType.NULL`. However, on the data node, `ReplaceFieldWithConstantOrNull` consults `searchStats.exists()` to decide whether to retain the field — and for flattened subfields, `exists()` returns `true` because they are physically mapped in Lucene. This causes the field to be retained and extracted with `ElementType.NULL`, leading to a type mismatch at the compute layer. ## Fix 1. In `ReplaceFieldWithConstantOrNull`, check for `MissingEsField` before consulting `searchStats.exists()`. If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats. 2. In `LateMaterializationPlanner`, run `ReplaceFieldWithConstantOrNull` in `toPhysical` so that null-typed fields are replaced with constant nulls before `InsertFieldExtraction` runs. This prevents the late materialization path from trying to extract them from the index. Resolves: elastic#142616.
Issue
When using
UNMAPPED_FIELDS="NULLIFY"with flattened field subfields (e.g.,resource.attributes.host.name), queries hit anIllegalStateException(element_type [BYTES_REF] NOT IN (NULL, NULL)) orUnsupportedOperationException(can't append non-null values to a null block).The coordinator correctly identifies flattened subfields as unmapped via field caps and marks them with
MissingEsField/DataType.NULL. However, on the data node,ReplaceFieldWithConstantOrNullconsultssearchStats.exists()to decide whether to retain the field — and for flattened subfields,exists()returnstruebecause they are physically mapped in Lucene.This causes the field to be retained and extracted with
ElementType.NULL, leading to a type mismatch at the compute layer.Fix
ReplaceFieldWithConstantOrNull, check forMissingEsFieldbefore consultingsearchStats.exists(). If the coordinator explicitly marked a field as missing, don't retain it regardless of local search stats.LateMaterializationPlanner, runReplaceFieldWithConstantOrNullintoPhysicalso that null-typed fields are replaced with constant nulls beforeInsertFieldExtractionruns. This prevents the late materialization path from trying to extract them from the index.Resolves: #142616.