Fix for Pandas merge overload wrong handling of string literals

[kozlov-alexey]

No description provided.

[shssf]

I'm not sure but I think these functions (including merge_overload) might be implemented easier. I mean, use if\else clause to calculate proper left_on and right_on variables and return the same _impl function.
At least, if I'm not mistaken, it will took lass code lines

[kozlov-alexey]

I tried that at first (i.e. assign new_right/left_on to proper values in context of merge_asof_overload() and remove lines 677 and 678 at all, hence using same body for _impl()), but for some reason it breaks currently working test_join1_seq_key_change1 (the only test we have currently to test "on=['A']" case). In other words, it looks like assignment:
left_on = right_on = on
only works for lists if it's in the context of _impl(). The error was as below:
KeyError: "Failed in hpat mode pipeline (step: convert to distributed)\nlist('A',)""
probably because the type of 'on' in context of merge_asof_overload() is not the same as type of 'on' in _impl(), hence providing one instead of the other causes failure.
So I ended up using two different overloads - legacy one for lists and new one taking literal values for strings.
Maybe there's a way to copy Numba's ConstList value 'literally' to new_left/right_on variables, but I don't know what it is.

[fschlimb]

Ignore my previous (deleted) comment.
What is the error of the original code?

[kozlov-alexey]

@fschlimb
The original error was "ValueError: Failed in hpat mode pipeline (step: typed dataframe pass)" in
get_const_or_list() in dataframe_pass.py. I'm attaching the file with full traceback here: https://github.com/IntelPython/hpat/files/3441340/full_traceback_for_test_join.txt

[fschlimb]

1. You should make sure you're using a compatible version of numpy
2. This means you have to understand why get_const_or_list() fails. Its name suggests it is intended to address exactly the issue you observe with literals/consts.

[kozlov-alexey]

@fschlimb

1. OK (it goes away with downgrading to scikit-learn=0.19.1 and numpy=1.16.1)
2. I believe the problem is in how Numba treats 'on' variable in the original merge implementation.
   That is, during Numba type inference 'on' becomes an instance of TypeVar class, so assigning later this value to left_on and right_on will not make them the same constant (we need to assign obj.literal_value for that). Maybe Numba fails to add const names 'left_on' and 'right_on' in IR when it copies Literal[Str] object?

And in _get_const_or_list() we merely try to find const (or lists) objects by name in self.func_ir, which fails due to above (no such const names exist in IR). I have no explanation why it works for lists though, probably, because they are created differently (via build sequence).

@overload_method(DataFrameType, 'merge')
@overload(pd.merge)
def merge_overload(left, right, how='inner', on=None, left_on=None,
right_on=None, left_index=False, right_index=False, sort=False,
suffixes=('_x', '_y'), copy=True, indicator=False, validate=None):

def _impl(left, right, how='inner', on=None, left_on=None,
        right_on=None, left_index=False, right_index=False, sort=False,
        suffixes=('_x', '_y'), copy=True, indicator=False, validate=None):
    if on is not None:
        left_on = right_on = on       # <-------- Even though 'on' was string literal, here we assign 
                                      # some Numba object, not object's literal value, hence left_on and 
                                      # right_on are not found as const objects in self.func_ir later
    return hpat.hiframes.api.join_dummy(
        left, right, left_on, right_on, how)

return _impl

[kozlov-alexey]

@fschlimb
Thank you for helping me sort that out and suggesting a better solution!
I had to modify the check a bit though, i.e. instead of
if isinstance(on, types.NoneType)
use
if isinstance(numba.typeof(on), types.NoneType)

because, with the first version other tests that do not provide 'on' argument fail.
It looks like that check always returns True. It may be because Numba's NoneType is not inherited from python's None:
numba/numba#3590

So we can either compare to NoneType but use typeof(on), as in this commit, or we can use the following negated check:
onHasValidType = isinstance(on, types.StringLiteral, types.List)
both of which appear to work.

[fschlimb]

The current check is probably better because it auto-allows for more types if!=None

[kozlov-alexey]

I had to skip back one of the tests - test_merge_asof_parallel1 - which still fails on NUM_PES=3 build now with a different error and only on the second test run. It can be seen in Travis logfile that it is passed on the first run though, so probably it's due to the problem with
test_series_head_index_parallel1
which is not skipped at the moment, but was identified as a root cause of corrupting memory and causing other test failures.

I'm going to deal with skipping test_series_head_index_parallel1 and unskipping other tests in PR#100 anyway, so I hope it's not a big issue.