Fix StoredFieldsSpec#merge(...) issue #138306
Conversation
If StoredFieldsSpec requires source and has no sourcePaths, then it requires the whole source. For example, in case of `SourceFieldBlockLoader`. When merging StoredFieldsSpec then omit the sourcePaths of the other StoredFieldsSpec. Closes elastic#138188
martijnvg
added
:StorageEngine/Mapping
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| mergedSourcePaths.addAll(other.sourcePaths); | ||
| } else if (this.sourcePaths.isEmpty() == false) { | ||
| mergedSourcePaths = this.sourcePaths; | ||
| if (other.requiresSource) { |
There was a problem hiding this comment.
Ah! So our intuition was right - every time we apply the filter, we are stripping down the rest of the source. The fix is to always keep the source if it's been asked for
There was a problem hiding this comment.
As a side note, there seems to be quite a bit of boiler plate in this method, with some repeated outcomes. Do we want to shove them into separate tiny inlinable functions?
There was a problem hiding this comment.
Let me see if that actually makes to code cleaner.
| index: my-index2 | ||
| body: | ||
| mappings: | ||
| dynamic: false |
There was a problem hiding this comment.
Is this important? The dynamic I mean
There was a problem hiding this comment.
For the query being tested here, yes. Otherwise the test wouldn't fail.
The FROM my-index2 METADATA _source | LIMIT 10 requests all mapped fields, but because baz isn't mapped without the fix to StoredFieldSpec, we would filter it out. If the baz field was mapped, the stored field spec of the other column would include it in the final stored field spec.
| - do: | ||
| esql.query: | ||
| body: | ||
| query: 'FROM my-index2 METADATA _source | LIMIT 10' |
There was a problem hiding this comment.
Not to be annoying, but are we testing here with what we actually want to be testing?
Here we seem to have an index with a single text field, so even if we had not fixed the bug, I think this still would've worked, right - we would've stripped the source of everything but the field, which doesn't actually change the _source.
Feels like we need the mapping with at least one another field, and the query that reads the entire source and the text field. We should be testing that the filtered out field is still returned in source
This thing - #138188 - has the correctly shaped example I think.
There was a problem hiding this comment.
Here we seem to have an index with a single text field, so even if we had not fixed the bug, I think this still would've worked, right - we would've stripped the source of everything but the field, which doesn't actually change the _source.
Without the fix to StoredFieldSpec this test fails.
I can add a @timestamp field, but that doesn't change the outcome of the test.
|
Also on the |
| spec = spec.merge( | ||
| new StoredFieldsSpec( | ||
| true, | ||
| false, |
There was a problem hiding this comment.
So this may be silly question, but it seems like we very much changed the meaning of this test, right? The way I read this, we changed requiresSource to false, which - in some sense - triggers the "pre-bug" behavior. This makes sense.
Should we add a mirror test with this flag set to true?
There was a problem hiding this comment.
The test just walks through a few scenarios and I just appended the scenario that triggered the bug. I can add a dedicated unit test for the bug here.
3 participants
If StoredFieldsSpec requires source and has no sourcePaths, then it requires the whole source. For example, in case of
SourceFieldBlockLoader. When merging StoredFieldsSpec then omit the sourcePaths of the other StoredFieldsSpec.Closes #138188
(a non released issue)