lucasbru commented on code in PR #19580: URL: https://github.com/apache/kafka/pull/19580#discussion_r2084543743
########## streams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamKTableLeftJoinTest.java: ########## @@ -45,7 +47,13 @@ import java.util.Random; import java.util.Set; +import static org.apache.kafka.common.utils.Utils.mkEntry; +import static org.apache.kafka.common.utils.Utils.mkMap; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; Review Comment: This is not decided on the project level, but I personally find it confusing that a few classes use hamcrest matchers and most others use JUnit5 basic assertions. I'd not introduce new classes using hamcrest. See also https://issues.apache.org/jira/browse/KAFKA-16929. But feel free to ignore since this more of a project-level concern to move away from hamcrest. ########## streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamKTableJoinProcessor.java: ########## @@ -150,7 +150,7 @@ private boolean maybeDropRecord(final Record<StreamKey, StreamValue> record) { // furthermore, on left/outer joins 'null' in ValueJoiner#apply() indicates a missing record -- // thus, to be consistent and to avoid ambiguous null semantics, null values are ignored final TableKey mappedKey = keyMapper.apply(record.key(), record.value()); - if (leftJoin && record.key() == null && record.value() != null) { + if (leftJoin && mappedKey == null && record.value() != null) { Review Comment: Could this problem also affect other join types? Have you checked? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
