arvindKandpal-ksolves commented on PR #28218: URL: https://github.com/apache/flink/pull/28218#issuecomment-4854185273
@snuyanzin thanks for the detailed explanation! We investigated thoroughly: **1. Is it a bug or optimization?** It is a deliberate Calcite optimization. In `SqlToRelConverter.java`, `isRemoveSortInSubQuery()` returns `true` by default — Calcite intentionally removes `ORDER BY` in a subquery without `LIMIT/FETCH` during SQL-to-rel conversion. So it is not a Calcite bug. **2. Should the same optimization be applied in Table API?** We checked — Flink already has `CoreRules.SORT_REMOVE` in `LOGICAL_RULES` for both batch and stream rule sets. However this rule only removes a sort when the input already satisfies the required collation — it does NOT eliminate a sort that is semantically dead before a global aggregate. There is currently no rule in Flink or Calcite that drops a dead `ORDER BY` before a global aggregate upstream in the Table API path. Adding such a rule would be a larger and separate change beyond the scope of this bug fix. **3. Why is the current fix acceptable?** The fix in `FlinkExpandConversionRule` is a minimal and safe guard — it prevents the planner crash without changing any optimization behavior. The bounds check `_.getFieldIndex < fieldCount` is generic and covers regular columns, metadata columns, and rowtime attributes equally. We are happy to open a follow-up ticket to track adding an upstream optimization that eliminates dead `ORDER BY` before global aggregate in the Table API path — if that is the preferred direction. *Note: Research and investigation for this PR was assisted using Claude Sonnet 4.6.* -- 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]
