weiqingy commented on PR #28218: URL: https://github.com/apache/flink/pull/28218#issuecomment-4673734332
Thanks for turning this around so thoroughly @arvindKandpal-ksolves — re-reviewed at `113593d` and the null guard in `satisfyTraitsByInput`, the `TableTestBase` + `verifyRelPlan` rewrite, the rename, and the safety comment on the `return null` all look right. The pinned plan confirms the dead `ORDER BY b` is dropped cleanly (no `Sort` in the optimized rel), which is exactly the outcome we want. On my earlier "eliminate the collation upstream" question: the localized trait-layer guard makes sense to me. The out-of-bounds collation is genuinely unsatisfiable, your comment justifies why dropping the path is safe, and tracking down where the trait should have been stripped upstream is a larger, riskier change for no behavioral gain. Let's keep the localized fix — consider that thread closed. One small thing worth confirming on the test: `onMatch` runs both `satisfyTraitsBySelf` and `satisfyTraitsByInput`, but the original crash reproduced on the `bySelf` path. Does `testOrderByWithGlobalAggregate` actually drive the `satisfyTraitsByInput` branch — where `batchRel.satisfyTraits(...)` returns `Some` — so the new guard at lines 93-95 gets exercised? If that branch isn't reachable for this query, no need to contort a test around it; the defensive null check stands on its own. I just want to make sure the line we added in direct response to the NPE finding isn't sitting uncovered. CI is green on this commit, so from my side this is close to ready. -- 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]
