Fix ignore_above for flattened fields
#112944
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
|
||
| if (entryValue instanceof List<?> valueAsList) { | ||
| for (Object value : valueAsList) { | ||
| if (value instanceof String valueAsString && valueAsString.length() < ignoreAbove) { |
There was a problem hiding this comment.
What can these be, other than String? Shall we assert?
There was a problem hiding this comment.
This is just to be extremely defensive...my expectation is that here we have String or List<String> but actually I see Object and List<Object>. Strings are objects, of course, but I need to cast them to count the number of characters and filter. There are also cases, anyway, where non-strings are expected. See for instance testFlattenedField in FieldFetcherTests. So, here we apply ignoreAbove filtering only on strings.
There was a problem hiding this comment.
What is that you want to assert? I still need to do the cast anyway...
There was a problem hiding this comment.
In that example, I see that we have a number so it's fine to ignore. I wonder if there are cases where we can have mixed singletons (e.g. strings) with maps pointing to more strings that can be ignored.
There was a problem hiding this comment.
I have added more tests covering cases I could imagine...I don't see anything missing.
|
|
||
| assertEquals(List.of(), fetchSourceValue(createDefaultFieldType(10), sourceValue)); | ||
| assertEquals(List.of(sourceValue), fetchSourceValue(createDefaultFieldType(20), sourceValue)); | ||
| } |
There was a problem hiding this comment.
Let's add some more elaborate unit tests, these are easier to debug.
| "key3", | ||
| "hi", | ||
| "key4", | ||
| List.of("the quick brown fox", "jumps over the lazy dog") |
There was a problem hiding this comment.
Can we add "key5", Map.of("nested", List.of("foo", "foobar")?
We can have subobjects, iiuc.
There was a problem hiding this comment.
I don't think so, I would expect a case like the one you described to be handled like
"key5.nested" -> ["foo", "foobar"]
because that is how flattening works, right?
I don't see any way for a user to put a map into a field.
key5.field1: foo
key5.field2: bar
key5.nested: ["foo", "foobar"]
which in json is
key5: {
field1: foo,
field2: bar,
nested: ["foo", "foobar"]
}
which is flattened as described above.
In a flattened field all values for each field are leaves...so non-nested stuff, that is either a single-value or a multi-value.
I mean we can do that in a test...but a user would never be able to do so...am I wrong?
There was a problem hiding this comment.
Yeah makes sense.. Let's add this to yaml for peace of mind, to verify the expected behavior.
Flattened fields do no actually use `ignore_above`. When retrieving source, `ignore_above` behaves as expected. Anyway, it is ignored when fetching field values through the fields API. In that case values are returned no matter the `ignore_above` setting. Here we implement the filtering logic in the `ValueFecther` implementation of `RootFlattenedFieldType`. (cherry picked from commit 91674f8)
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
* Fix `ignore_above` for flattened fields (#112944) Flattened fields do no actually use `ignore_above`. When retrieving source, `ignore_above` behaves as expected. Anyway, it is ignored when fetching field values through the fields API. In that case values are returned no matter the `ignore_above` setting. Here we implement the filtering logic in the `ValueFecther` implementation of `RootFlattenedFieldType`. (cherry picked from commit 91674f8) * Fix --------- Co-authored-by: Salvatore Campagna <93581129+salvatore-campagna@users.noreply.github.com>
4 participants
Flattened fields do no actually use
ignore_above. When retrieving source,ignore_abovebehavesas expected. Anyway, it is ignored when fetching field values through the fields API. In that case
values are returned no matter the
ignore_abovesetting. Here we implement the filtering logicin the
ValueFectherimplementation ofRootFlattenedFieldType.