Added null check for ParentJoinFieldMapper #42997

[0xamogh]

#42997

[polyfractal]

Great, thanks @amogh-jrules! Left a comment about the change.

Would you be willing to add a unit test for this too, to make sure the unmapped case works as expected? I think ParentToChildrenAggregatorTests would be the right place to add a test. Let me know if you have questions, parent/child tests are a little more complicated than elsewhere. You may also need #43405 to merge first so that testing unmapped fields is a little easier.

Thanks again for the PR!

[polyfractal]

Hmm, let's see if we can un-nest the if conditional? Usually cleaner to read if they don't nest in my opinion. How about something like:

if (parentJoinFieldMapper == null) {
    config.unmapped(true);
    return;
}

ParentIdFieldMapper parentIdFieldMapper = parentJoinFieldMapper.getParentIdFieldMapper(childType, false);
if (parentIdFieldMapper == null) {
    config.unmapped(true);
    return;
}

And then after that point we know both fields are not null, so we can set the config outside of a conditional. Should flatten the whole thing a bit.

[0xamogh]

Alright, makes sense

[elasticmachine]

Pinging @elastic/es-analytics-geo

[0xamogh]

Great, thanks @amogh-jrules! Left a comment about the change.

Would you be willing to add a unit test for this too, to make sure the unmapped case works as expected? I think ParentToChildrenAggregatorTests would be the right place to add a test. Let me know if you have questions, parent/child tests are a little more complicated than elsewhere. You may also need #43405 to merge first so that testing unmapped fields is a little easier.

Thanks again for the PR!

Okay, I haven't done this before, I'll give it a shot, please help me out if I mess up

[polyfractal]

No problem at all, here to help! Feel free to ping with questions, or push a work-in-progress and I can leave comments/suggestions.

[0xamogh]

Hey @polyfractal , I have flattened the if-else structure and added the commit. Also, I took a look at creating a unit test. I had 1 question though. In essence, what I have to do is create a new method in the ParentToChildrenAggregatorTests file which calls this particular case when parentIdFieldMapper is null. Is that correct? As in went through the file I couldn't quite find a method which calls .getMapper() method as in the file I changed

[polyfractal]

Hey @amogh-jrules sorry for the delay, saw this and then forgot to reply.

In essence, what I have to do is create a new method in the ParentToChildrenAggregatorTests file which calls this particular case when parentIdFieldMapper is null. Is that correct?

I haven't looked super closely, but that sounds correct to me. You should be able to just set the field type to null (or pass in null directly). You can see the field type being passed in here. Setting this to null will tell the test that the field is unmapped.

You'll have to tweak the testCase() method to allow passing in a field type, similar to what was done in #43405 (adding another overload of testCase which accepts a field type)

[0xamogh]

Thanks for your reply, I'll get on it right away

[elasticsearchmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)