feat: add NestedLoopJoinRel definition #561
Conversation
danepitkin
requested review from
jacques-n,
cpcloud and
westonpace
as code owners
proto/substrait/algebra.proto
Outdated
| @@ -433,6 +433,7 @@ message Rel { | |||
| CrossRel cross = 12; | |||
| //Physical relations | |||
| HashJoinRel hash_join = 13; | |||
| NestedLoopJoinRel nested_loop_join = 18; | |||
There was a problem hiding this comment.
Given that this is the third defined join relation perhaps it should come after MergeJoinRel (here in Rel and as defined later)?
There was a problem hiding this comment.
Good point! I don't know why I put it in the middle..
danepitkin
force-pushed
the
danepitkin/nestedloopjoin
branch
from
7b861d4 to
ae534eb
Compare
There was a problem hiding this comment.
Do you want to also add this to the markdown? It can go on the physical relations page next to the hash equijoin.
| Rel left = 2; | ||
| Rel right = 3; | ||
| Expression expression = 4; | ||
|
|
There was a problem hiding this comment.
Should you add post_join_filter?
There was a problem hiding this comment.
According to the existing NLJ definition, it doesn't require one. I think that makes sense because anything that goes in a post_join_filter could also just be included in the expression IIUC.
There was a problem hiding this comment.
Good point. I believe you are correct.
There was a problem hiding this comment.
Although this same point would apply to the logical operator as well 🤔
There was a problem hiding this comment.
Haha, interesting... 😅
| JOIN_TYPE_LEFT_ANTI = 7; | ||
| JOIN_TYPE_RIGHT_ANTI = 8; |
There was a problem hiding this comment.
I historically don't like anti joins because they are not well defined.
For example, a semi join can be implemented as:
for l_row in left:
for r_row in right:
result = expression(l_row, r_row):
if result == True:
yield l_row
However, when we look at the anti join:
for l_row in left:
for r_row in right:
result = expression(l_row, r_row)
if ???:
yield l_row
Should ??? be result == False or should it be result != True? Specifically the question is around how we handle the case where result is null.
Do you want to take a stab at either picking one of these and clearly documenting this or adding a new proto field allowing the user to choose between these two behaviors?
That being said, I won't hold this issue up over this, so if you'd rather punt on the question then that is fine too.
There was a problem hiding this comment.
Very illustrative point, thank you. I am in favor of allowing the user to define NULL behavior for the anti-join. However, I would prefer to push this to a separate PR where JoinType is refactored out for all of NLJ/Hash/SM operators (I assume this should be applied to all three). I can create a separate issue for that if you think it's appropriate?
There was a problem hiding this comment.
Hey @westonpace! Thanks for the review 😁
The NLJ operator is already defined in the markdown: https://substrait.io/relations/physical_relations/#nlj-operator
Is there anything you'd change about it?
| Rel left = 2; | ||
| Rel right = 3; | ||
| Expression expression = 4; | ||
|
|
There was a problem hiding this comment.
According to the existing NLJ definition, it doesn't require one. I think that makes sense because anything that goes in a post_join_filter could also just be included in the expression IIUC.
| JOIN_TYPE_LEFT_ANTI = 7; | ||
| JOIN_TYPE_RIGHT_ANTI = 8; |
There was a problem hiding this comment.
Very illustrative point, thank you. I am in favor of allowing the user to define NULL behavior for the anti-join. However, I would prefer to push this to a separate PR where JoinType is refactored out for all of NLJ/Hash/SM operators (I assume this should be applied to all three). I can create a separate issue for that if you think it's appropriate?
Co-authored-by: Weston Pace <weston.pace@gmail.com>
danepitkin
dismissed stale reviews from westonpace and EpsilonPrime
via
2e716db
|
This PR has 3 SMC votes. Will merge it. |
This PR adds the NestedLoopJoinRel proto definition.
Closes #456