Remove the 'array value parser' marker interface. #57571
Conversation
jtibshirani
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (:Search/Mapping) |
This PR replaces the marker interface with the method FieldMapper#parsesArrayValue. I find this cleaner and it will help with the fields retrieval work (elastic#55363). The refactor also ensures that only field mappers can declare they parse array values. Previously other types like ObjectMapper could implement the marker interface and be passed array values, which doesn't make sense.
jtibshirani
force-pushed
the
parse-array-values
branch
from
4b2dfdb to
2932918
Compare
| @@ -562,6 +562,10 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper, | |||
| } | |||
| } | |||
|
|
|||
| private static boolean parsesArrayValue(Mapper mapper) { | |||
| return mapper instanceof FieldMapper && ((FieldMapper) mapper).parsesArrayValue(); | |||
There was a problem hiding this comment.
Unfortunately this refactor didn't actually cut down on the number of instanceof checks. But I feel better about this one, because it's very common for us to check instanceof against FieldMapper or ObjectMapper -- it's just part of how mappings + document parsing currently work.
|
I think I'd prefer the method on Mapper. Or *something* with tighter typing
to avoid the instanceof FieldMapper bit. I dunno. I feel like adding an
instanceof to remove and instanceof might be a sign of something bad.
…On Tue, Jun 2, 2020, 19:48 Julie Tibshirani ***@***.***> wrote:
@jtibshirani <https://github.com/jtibshirani> requested your review on:
#57571 <#57571> Remove the
'array value parser' marker interface..
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#57571 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABUXIUSQE7YD55G4OEQVWTRUWFTFANCNFSM4NRFM2YQ>
.
|
|
And our comments crossed, like ships in the night.
Is there anything we can do to cut down on the instanceof ObjectMapper vs
FieldMapper? Like, in a follow up?
…On Tue, Jun 2, 2020, 20:00 Nikolas Everett ***@***.***> wrote:
I think I'd prefer the method on Mapper. Or *something* with tighter
typing to avoid the instanceof FieldMapper bit. I dunno. I feel like adding
an instanceof to remove and instanceof might be a sign of something bad.
On Tue, Jun 2, 2020, 19:48 Julie Tibshirani ***@***.***>
wrote:
> @jtibshirani <https://github.com/jtibshirani> requested your review on:
> #57571 <#57571> Remove the
> 'array value parser' marker interface..
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#57571 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABUXIUSQE7YD55G4OEQVWTRUWFTFANCNFSM4NRFM2YQ>
> .
>
|
|
We could put the method on
I will give this some thought -- I'll likely file an issue about it so we don't forget. I see it as a worthwhile but bigger and separate refactor from this one. |
|
@elasticmachine run elasticsearch-ci/1 |
👍 |
| @@ -173,6 +172,11 @@ public DenseVectorFieldType fieldType() { | |||
| return (DenseVectorFieldType) super.fieldType(); | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean parsesArrayValue() { | |||
There was a problem hiding this comment.
Should this be final, like AbstractGeometryFieldMapper does?
There was a problem hiding this comment.
To me this could be a bit confusing, because no other methods on DenseVectorFieldType are final and it wasn't designed for extension.
I also found this refactor to be disappointing on its own. But I think we're slowly pushing the document parsing + field mapper code in a good direction. |
|
Thank you both for the review ! |
This PR replaces the marker interface with the method FieldMapper#parsesArrayValue. I find this cleaner and it will help with the fields retrieval work (elastic#55363). The refactor also ensures that only field mappers can declare they parse array values. Previously other types like ObjectMapper could implement the marker interface and be passed array values, which doesn't make sense.
This PR replaces the marker interface with the method FieldMapper#parsesArrayValue. I find this cleaner and it will help with the fields retrieval work (elastic#55363). The refactor also ensures that only field mappers can declare they parse array values. Previously other types like ObjectMapper could implement the marker interface and be passed array values, which doesn't make sense.
This PR replaces the marker interface with the method FieldMapper#parsesArrayValue. I find this cleaner and it will help with the fields retrieval work (#55363). The refactor also ensures that only field mappers can declare they parse array values. Previously other types like ObjectMapper could implement the marker interface and be passed array values, which doesn't make sense.
This PR replaces the marker interface with the method FieldMapper#parsesArrayValue. I find this cleaner and it will help with the fields retrieval work (#55363). The refactor also ensures that only field mappers can declare they parse array values. Previously other types like ObjectMapper could implement the marker interface and be passed array values, which doesn't make sense.
This PR replaces the marker interface with the method
FieldMapper#parsesArrayValue. I find this cleaner and it will help cut downon
instanceofchecks in the fields retrieval work (#55363).The refactor also ensures that only field mappers can declare they parse array
values. Previously other types like
ObjectMappercould implement the markerinterface and be passed array values, which doesn't make sense.