lzcGeek commented on issue #39926: URL: https://github.com/apache/shardingsphere/issues/39926#issuecomment-5824291744
I would like to work on this issue. Root cause is two-fold, and I confirmed both locally against `master`: 1. `FirebirdDMLStatementVisitor.visitSelectClause` only handles `ctx.firstSkipClause()`; the `limitClause?` that `selectClause` carries (`DMLStatement.g4:80`) has no visit, so `OFFSET ... ROWS FETCH FIRST ... ROWS ONLY` never produces a `LimitSegment` and the rewrite rule sees no offset. 2. `orderByClause` also carries a trailing `limitClause?` (`BaseRule.g4:393`). ANTLR's greedy prediction consumes the clause inside `orderByClause`, so for the SQL in the report (`... ORDER BY ID OFFSET 100000 ROWS FETCH FIRST 10 ROWS ONLY`) `ctx.limitClause()` is null at the `selectClause` level. Since `orderByClause` is reused by window specifications (`OVER (...)`), `GROUP_CONCAT(...)` and friends, accepting a LIMIT there is wrong anyway, and every statement that wants one already declares its own `limitClause?` (DML/DAL/DDL). So the fix is to drop it from `orderByClause`. Plan, if that direction is acceptable: - add `visitLimitClause` to `FirebirdDMLStatementVisitor` and wire it into `visitSelectClause` - remove the nested `limitClause?` from `orderByClause` in the Firebird `BaseRule.g4` - add the two missing IT cases: `SELECT * FROM t_order OFFSET 100000 ROWS FETCH FIRST 10 ROWS ONLY` and the `ORDER BY` variant from the report, with assertions on `offset` / `rowCount` - note that the existing `select_pagination_with_offset_fetch` case is `db-types="SQLServer,PostgreSQL,openGauss"` and never included Firebird, which is why this was not caught Status of local verification: I applied both parts and ran the Firebird parser IT locally - `InternalFirebirdParserIT` passes 238/238, and with only one of the two parts applied the `ORDER BY` case fails, so both are needed. I also checked `SELECT * FROM t_order ROWS 100000 TO 100009` and found the `rowsClause` branch of `visitLimitClause` does not resolve its operands (both come out null) and that Firebird's `ROWS m TO n` is an inclusive range, so the conversion would be `offset=m, rowCount=n-m+1`. That grammar is not covered by any existing case; I will keep it out of this PR to keep the change focused and file it separately. I have the patch ready and can open the PR. -- 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]
