MINOR: Change KStreamKstreamOuterJoinTest to use distinct left and right types#15513
Conversation
Signed-off-by: Greg Harris <greg.harris@aiven.io>
cadonna
left a comment
There was a problem hiding this comment.
Thanks for the change, @gharris1727 !
I have one comment.
| new KeyValueTimestamp<>(0, "C0+0", 0L), | ||
| new KeyValueTimestamp<>(0, "C0+0", 0L), | ||
| new KeyValueTimestamp<>(1, "C1+1", 0L), | ||
| new KeyValueTimestamp<>(1, "C1+1", 0L), |
There was a problem hiding this comment.
This seems to make the verification less strict. It does not differentiate between a0 and b0 as well as a1 and b1. Maybe use 10 for a0 and 20 for b0 and 11 for a1 and 21 for b1.
There was a problem hiding this comment.
You're right, I didn't notice this. I did a search-and-replace renaming, and reverted the stuff which didn't make sense.
I did have to manually renumber stuff like "a0-0", and some places where capital letters "A0" were used on the inputStream2 to fit the pattern better. PTAL, thanks!
Signed-off-by: Greg Harris <greg.harris@aiven.io>
|
@gharris1727 @cadonna -- do we still want to merge this PR? |
|
Yes, why not? It is a good change to the test, no? |
|
Was just wondering if the other PR contained tests that cover this already. @gharris1727 could you rebase so a new PR build is triggered to make sure everything still works? Happy to reveiw/merge afterwards. |
|
@mjsax It was already up-to-date without merge conflicts, but I synced it up again. |
|
Thanks @gharris1727 -- merged to |
This test uses the same value types on the left and right, and so wouldn't be sensitive to a mixup between left and right values. So I changed one of the stream types to
Long, and updated the assertions to match.Because the implementation of the KStreamKstreamJoin is not type-safe, it was unclear just from the code that a mixup was not possible.
Committer Checklist (excluded from commit message)