[CALCITE-2591] EnumerableDefaults#mergeJoin should throw error and not return incorrect results when inputs are not ordered#859
Conversation
eolivelli
changed the title
…t return incorrect results when inputs are not ordered
eolivelli
force-pushed
the
fix/CALCITE-2591-no-assert
branch
from
e5420ce to
14f8b5c
Compare
| assert !generateNullsOnLeft : "not implemented"; | ||
| assert !generateNullsOnRight : "not implemented"; | ||
| if (generateNullsOnLeft) { | ||
| throw new UnsupportedOperationException("not implemented"); |
There was a problem hiding this comment.
Please use meaningful exception messages.
Do you really want to see UnsupportedOperationException("not implemented") in production logs? I bet you don't
|
@vlsi fixed the message, please take another look |
| int c = leftKey.compareTo(leftKey2); | ||
| if (c != 0) { | ||
| assert c < 0 : "not sorted"; | ||
| if (c < 0) { |
There was a problem hiding this comment.
The condition should be c > 0.
Original assertion was c<0==true, and assertion is violated in c>0 case.
There was a problem hiding this comment.
oh, yes, sorry, trivial error !!!
So it should be "c >= 0" strictly speaking
There was a problem hiding this comment.
c==0 is acceptable (merge join allows to have duplicate values in the input)
| assert c < 0 : "not sorted"; | ||
| if (c < 0) { | ||
| throw new IllegalStateException( | ||
| "not sorted, " + leftKey + " is less than " + leftKey2); |
There was a problem hiding this comment.
The message should be updated to account c>0.
A bit more verbose message won't hurt.
For instance: "mergeJoin assumes sorted in ascending order input, however ... "
| if (c > 0) { | ||
| throw new IllegalStateException( | ||
| "mergeJoin assumes sorted in ascending order input, " | ||
| + "however " + leftKey + " is less than " + leftKey2); |
There was a problem hiding this comment.
leftKey + " is less than " + leftKey2 seems to be wrong.
c = leftKey.compareTo(leftKey2).
c > 0 ==> leftKey > leftKey2
There was a problem hiding this comment.
yes, sure, I wrote the comment when the assertion was wrong :-)
sorry
|
Thank you @vlsi for your quick review I have applied your suggestions |
|
LGTM. Will commit if no-one objects in a day or two. |
|
Thank you very much. |
…t return incorrect results when inputs are not ordered (Enrico Olivelli) fixes apache#859
…t return incorrect results when inputs are not ordered (Enrico Olivelli) fixes apache#859
…t return incorrect results when inputs are not ordered (Enrico Olivelli) fixes apache#859 Change-Id: Iab1f286779b0fda255d84ff5111d327cb55cbdd8
2 participants
No description provided.