terrymanu commented on PR #39229: URL: https://github.com/apache/shardingsphere/pull/39229#issuecomment-5066294519
### Summary **Review Result: Mergeable** Reason: This conclusion covers code scope only; CI not reviewed by request. The latest head, `86a1d4872dfe7be2f8183efaca0853cb03f50ba3`, fixes the actual root cause and adds regression coverage for the SQL Server delimited-alias path from parser to binder. No code-level blockers were found. ### Evidence - `SQLServerFunctionOption.java:31-32` narrows the unparenthesized-function set to exactly `CURRENT_TIMESTAMP`, `CURRENT_USER`, `SESSION_USER`, `SYSTEM_USER`, and `USER`. Therefore, `ColumnSegmentBinder` no longer incorrectly skips ordinary columns named `ROWNUM`, `ROWNUM_`, or `ROW_NUMBER`. - The official SQL Server syntax is `ROW_NUMBER ( ) OVER (...)`. The repository’s `BaseRule.g4:403-405` likewise models it as `rowNumberFunction`, and the ordinary expression path produces a `FunctionSegment` without relying on the unparenthesized-function list. [Microsoft ROW_NUMBER documentation](https://learn.microsoft.com/en-us/sql/t-sql/functions/row-number-transact-sql?view=sql-server-ver17) - `SQLServerStatementVisitor.java:1924-1931` now normalizes TOP projection aliases through the existing `visitAlias` path, preventing `[rownum_]` from being stored verbatim as the projection label. - `TopProjectionSegment.java:41-42` returns the normalized alias as the column label when one is present. A missing or empty alias still returns the previous `TOP` fallback, preserving existing behavior. - The new BinderIT case covers `ROW_NUMBER() AS [rownum_]`, the outer `row_.[rownum_]` projection, and the pagination predicate. Its expected model confirms that the alias is consistently normalized to `rownum_`. Brackets are officially supported SQL Server identifier delimiters. [Microsoft database identifiers documentation](https://learn.microsoft.com/en-us/sql/relational-databases/databases/database-identifiers?view=sql-server-ver17) - `ROW_NUMBER` remains in `DialectPaginationOption` for pagination detection. That responsibility is independent of `DialectFunctionOption`, which describes unparenthesized functions, so it does not justify retaining the incorrect function metadata. - The only production construction sites for `TopProjectionSegment` are in the SQL Server visitor; no behavior change was found for other dialect parsers. ### Review Details - Review Focus: Code Correctness Review — CI not reviewed by request. - Reviewed Scope: Latest head `86a1d4872dfe7be2f8183efaca0853cb03f50ba3`; base/merge-base `6bb19309985071fdae6f94c020ca3ae1c8a4356a`; the local triple-dot file list exactly matched all 7 files reported by GitHub’s `/pulls/39229/files` endpoint. The review covered SQL Server function metadata, grammar, visitor logic, `ColumnSegmentBinder`, TOP/ROW_NUMBER pagination engines, projection-label binding, related unit tests, and the data-driven BinderIT. Related SQL92/Oracle metadata and pagination responsibilities were compared, and the existing SQL Server parser/optimizer pagination baselines were checked. - Not Reviewed Scope: GitHub Actions, check runs, execution against a real SQL Server instance, and modules outside the PR scope. - Verification: - `SQLServerFunctionOptionTest` and `TopProjectionSegmentTest`: exit `0`; 4 tests passed. - `SQLServerBinderIT` using the current-head reactor build: exit `0`; 4 tests passed. `-am` was used to ensure the SQL Server parser-engine dependency came from the latest PR source. - `TopPaginationContextEngineTest`, `PaginationContextEngineTest`, and `FunctionExpressionSegmentBinderTest`: exit `0`; 14 tests passed. - Release Note / User Docs: Not required. This is an internal parser/binder metadata correction and does not change configuration, public APIs, SPIs, or user workflows. -- 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]
