Revert change (#90674)#103865
Conversation
benwtrent
added
>bug
:Search Foundations/Mapping
|
Hi @benwtrent, I've created a changelog YAML for you. |
|
Pinging @elastic/es-search (Team:Search) |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I went through this and compared line by line with the PR that it reverts. It looks good to me. I think it is wise to revert at this point, and reconsider what we want to do next. You mentioned it did not revert cleanly: did the problems only come from the float handling in postProcessDynamicArrayMapping for dynamic mapping of vector fields?
Thanks for jumping on this!
Correct, the logic there changed from a separate bug fix. |
benwtrent
added
auto-merge-without-approval
|
@elasticmachine update branch |
benwtrent
removed
the
auto-merge-without-approval
…lastic#103047)" This reverts commit d6e8217.
benwtrent
added
the
auto-merge-without-approval
|
@elasticmachine update branch |
| new HashMap<>(), | ||
| new HashSet<>(), | ||
| new LinkedHashMap<>(), | ||
| new HashMap<>(), |
There was a problem hiding this comment.
++ it makes sense to go back to the original hashmap, which we had before.
|
run elasticsearch-ci/part-2 |
|
run elasticsearch-ci/part-1 |
|
FWIW, I'm also happy about this change as it will make #102936 and #96235 much easier as we don't need separate implementations for Before we re-introduce this change (in case we want to do that at some point), we should really merge the two PRs mentioned above first. |
By "re-introduce" you mean adding back "keep builders" change that this PR reverts? |
|
@elasticmachine update branch |
Yes, that's what I meant. Aka revert the revert. So before we're considering to re-introduce to "Store dynamic mapping updates as builders", we should first merge #102936 and #96235. We can then weigh in if the additional complexity of maintaining a |
Reverts elastic#90674 The revert is not perfectly clean as there are some minor adjustments to account for later changes. This is in contrast with: elastic#103858 closes: elastic#103011
Reverts elastic#90674 The revert is not perfectly clean as there are some minor adjustments to account for later changes. This is in contrast with: elastic#103858 closes: elastic#103011
Because of elastic#103865, DocumentParserContext#addDynamicMapper receives a Mapper, not a Mapper.Builder again. Therefore, we don't need a mapperSize method for the builder. This simplifies things a lot.
Because of elastic#103865, DocumentParserContext#addDynamicMapper receives a Mapper, not a Mapper.Builder again. Therefore, we don't need a mapperSize method for the builder. This simplifies things a lot.
Today, we're counting all mappers, including mappers for subfields that aren't explicitly added to the mapping towards the field limit. This means that some field types, such as `search_as_you_type` or `percolator` count as more than one field even though that's not apparent to users as they're just defining them as a single field in the mapping. This change makes it so that each field mapper only counts as one. We're still counting multi-fields. This makes it easier to understand for users why the field limit is hit. ~In addition to that, it also simplifies #96235 as it makes the implementation of `Mapper.Builder#getTotalFieldsCount` much easier and easier to align with `Mapper#getTotalFieldsCount`. This reduces the risk of over- or under-estimating the field count of a `Mapper.Builder` in `DocumentParserContext#addDynamicMapper`, which in turn reduces the risk of data loss due to the issue described here: #96235 (comment) *Edit: due to #103865, we don't need an implementation of `getTotalFieldsCount` or `mapperSize` in `Mapper.Builder`. Still, this PR more closely aligns `Mapper#getTotalFieldsCount` with `MappingLookup#getTotalFieldsCount`, which `DocumentParserContext#addDynamicMapper` uses to determine whether the field limit is hit* A potential risk of this is that we're now effectively allowing more fields in the mapping. It may be surprising to users that more fields can be added to a mapping. Although, I'd not expect negative consequences from that. Generally, I'd expect users to be happy about any change that reduces the risk of data loss. We could also think about whether to apply the new counting logic only to new indices (depending on the `IndexVersion`). However, that would add more complexity and I'm not convinced about the value. We'd then need to maintain two different ways of counting fields and also require passing in the `IndexVersion` to `MappingLookup` which previously didn't require the `IndexVersion`. This PR is meant as a conversation starter. It would also simplify #96235 but I don't think this blocks that PR in any way. I'm curious about the opinion of @javanna and @jpountz on this.
5 participants
Reverts #90674
The revert is not perfectly clean as there are some minor adjustments to account for later changes.
This is in contrast with: #103858
closes: #103011