gustavodemorais commented on PR #29570: URL: https://github.com/apache/flink/pull/29570#issuecomment-6502843502
Thanks @rionmonster! Looking at the whole diff again after you addressed the comments, the operator code has become much harder to reason about, and I'm hesitant to go further in this direction. I think we went too far in the last rounds, partly because of my own comments. I'd suggest we keep this PR to the deterministic bugs: an upsert counted twice, lost non-equi matches, key-only deletes, and the replaced left row. I hoped the code wouldn't get this complicated, but the complexity we added for cases that the engine handles differently elsewhere is probably not worth it for now. So could we drop the handling for cases that depend on state TTL, state restored from older versions, or tombstones of keys that were never inserted? That's the left retraction lookup and skip (:157-176), the right `mayBeNotStored` skip (:276-287) with `mayDeleteRecordsThatAreNotStored` and `leftIsUpsert`/`rightIsUpsert`, the `|| joinKeyContainsUniqueKey(false)` in `removeMatch`, and their tests. Please let me know if you think some of these are indeed important and should stay! The regular join has the same cases, so we can handle them together in FLINK-41258. Sorry for the back and forth! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
