InMemoryDatasets.jl icon indicating copy to clipboard operation
InMemoryDatasets.jl copied to clipboard

Add the inequalities to the leftjoin() and leftjoin!() functions.

Open kyo227 opened this issue 3 years ago • 6 comments

I refer to the inequality that implementation on the on parameter in innerjoin() function, then implemented it on the leftjoin() and leftjoin!() function.
Now, we can use leftjoin like this: leftjoin(dsl,dsr,on = [1=>1, 2=>(:lower,nothing)]). The methods used to find ranges and idx are the same as the methods in innerjoin, i just modified the way that uses ranges and idx to generate the result dataset in leftjoin(). I have done some testing, and this function works fine, and the run time and allocations are similar with innerjoin().

kyo227 avatar Sep 08 '22 14:09 kyo227

Please add tests for you functions. test/join.jl has a test set for range join, you may put your tests there.

sl-solution avatar Sep 09 '22 06:09 sl-solution

I've added tests to test/join.jl and fixed some bugs.

kyo227 avatar Sep 12 '22 15:09 kyo227

This is a good PR, however, I have some concerns about the way you implemented it. General feedback:

  • There are lots of code repeats
  • You should use innerjoin implementation. You may put all core computation of innerjoin in a new function which can handle both inner and left joins.
  • leftjoin and innerjoin are basically the same, except that innerjoin drops rows with no match (these rows have inbits = false)
  • There are no test for leftjoin! - the question is do we need range join for leftjoin!? I added some comments for some parts of the code.

sl-solution avatar Sep 13 '22 06:09 sl-solution

Thank you for your advice. It seems that my code still has some problems. In this part, I forgot to change this function. So ,the test will not pass. image

kyo227 avatar Sep 13 '22 07:09 kyo227

I made changes to the implementation of range join in leftjoin and it passed the test in test/join.jl.

kyo227 avatar Sep 20 '22 14:09 kyo227

Sorry, I found some problems in my code. This code is not ready yet.

kyo227 avatar Sep 22 '22 05:09 kyo227