Fix optimization of queries containing metrics withfill_nulls_with set
#1963
Conversation
…with non-conflicting null-fill values.
tlento
left a comment
tlento
left a comment
There was a problem hiding this comment.
See inlines. My big questions are:
- Are we sure this fix doesn't cause other issues where maybe we fill nulls when we shouldn't?
- If we are sure, can we add a simple direct test case somehow and document the expected behavior via test enforcement? I know visitors are almost impossible to test directly in this way, but maybe we can do something. Absent that a docstring will suffice.
| combined_parent_node = combined_parent_nodes[0] | ||
|
|
||
| if self._current_left_node.null_fill_value_mapping != current_right_node.null_fill_value_mapping: | ||
| if self._current_left_node.null_fill_value_mapping.has_conflict(current_right_node.null_fill_value_mapping): |
There was a problem hiding this comment.
This is incredibly subtle. I'm assuming the behavior change here is the fix. If that's wrong then I think we've got more explaining to do.
If that's right, we need better documentation of why it is this way. An outsider (or even someone like me, and probably you as well when you first wrote this) would assume that the original equality check is what we should be doing here, because we don't want to merge metric input nodes that do null fills with metric input nodes that do not.
Except it seems like we actually DO want to do this. Why? Is this because null fill happens after the point where these branches are merging? And is that always true?
Assuming this is the fix, then at a minimum I'd want a docstring on this method explaining what the function is meant to do. Ideally we'd have some kind of test case illustrating the behavior as well - the snapshot cases aren't adequate, because they're too high level for this kind of detail behavior, but an errant change to this behavior will just break everything again (or newly, in different subtle but annoying ways).
This PR addresses the case in #1962.