ganeshashree commented on PR #58238: URL: https://github.com/apache/spark/pull/58238#issuecomment-5409036421
> My apologizes. I changed my mind - the fix of watermark placement is much more complicated than it seems and it requires focused effort to get this done properly. I feel like the contribution is LLM driven and it's taking much more time to review than I could prompt, unfortunately. (I only partially reviewed the code and the characteristic of the doc change is purely what I have been seen from LLM. I'm not saying we should not generate the sentence from LLM but that's always unnecessary verbose where we can prompt further to be concise and brief - I suspect this change was self-reviewed.) > > I'm OK with simply adding LeftAnti join, leaving the existing watermark placement issue as it is (and even expand that problem to LeftAnti) and asking folks (or myself) who are more expert on this subject to revisit it. > > @ganeshashree Sorry for back and forth. I'd love to make the code change be more thoughtful and more time and effort is given to make it better since the complexity for StreamingJoinHelper is already outstanding. Let's step back a bit and just make a simpler change to introduce LeftAnti join. I wouldn't ask to solve this issue in your PR of LeftAnti join; just extend the issue to LeftAnti join as same as LeftSemi join. Thanks @HeartSaVioR, that makes sense. I'd rather get this right than rush it in. For the record, what this PR did: it added a shared analyzer check, `checkStreamStreamJoinWatermarkPlacement`, enforcing two requirements for left semi and left outer, left state eviction (the equi eviction key must be watermarked, or the range bound must sit between watermarked attributes on both sides), and for left outer only, both eviction ordinal join keys watermarked so a late row cannot invalidate an already emitted unmatched row. The helpers mirror the runtime's own attribute selection, and it was gated behind a kill switch. That logic can carry into the focused effort below. I'll take the redirect: - Drop the watermark placement fix from this line of work. I'll close this PR and keep the LeftAnti change in #57813 minimal, just extending the existing behavior to LeftAnti the same way it works for LeftSemi today, without touching placement. - Park the placement fix as its own focused effort. Since it touches a critical area, this is best done by an SS expert. On the docs: fair point, and taken. I'll keep user-facing docs to the level of detail already in the guide, move the config out of the guide (migration guide only), mark it `.internal()`, and use your migration-guide wording. I'll tighten the prose by hand rather than leaving it verbose. Sorry for the extra review churn here. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
