014-code commented on PR #39237: URL: https://github.com/apache/shardingsphere/pull/39237#issuecomment-5104642491
> ### Summary > **Review Result: Not Mergeable** > > **Feedback Mode: Change Request** > > **Reason:** The reported top-level MySQL `LIMIT ?` case is fixed, but the same `Long`-to-`Integer` failure remains for pagination parameters owned by nested SELECTs. > > ### Issues > #### P1: Normalize pagination parameters below the root SELECT > * **Problem:** [`createParameters`](https://github.com/apache/shardingsphere/blob/f6b60b9da539c0d31ab021ab1178223089a5b273/kernel/sql-federation/core/src/main/java/org/apache/shardingsphere/sqlfederation/engine/processor/impl/StandardSQLFederationProcessor.java#L151-L183) obtains only the root `SelectStatementContext` pagination indexes. That context is built from the root statement alone in [`SelectStatementBindingContext`](https://github.com/apache/shardingsphere/blob/f6b60b9da539c0d31ab021ab1178223089a5b273/infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/SelectStatementBindingContext.java#L41-L46). SQL Federation nevertheless recursively converts child SELECTs, including derived tables and combine branches, through [`SubqueryTableConverter`](https://github.com/apache/shardingsphere/blob/f6b60b9da539c0d31ab021ab1178223089a5b273/kernel/sql-federation/compiler/src/main/java/org/apache/shardingsphere/sqlfederation/compiler/sql/ast /converter/segment/from/impl/SubqueryTableConverter.java#L50-L60) and [`SelectStatementConverter`](https://github.com/apache/shardingsphere/blob/f6b60b9da539c0d31ab021ab1178223089a5b273/kernel/sql-federation/compiler/src/main/java/org/apache/shardingsphere/sqlfederation/compiler/sql/ast/converter/statement/type/SelectStatementConverter.java#L51-L60). A `Long` bound to a child `LIMIT ?` is therefore not converted. > * **Impact:** Calcite 1.40.0 applies the same `Integer` conversion to every dynamic `EnumerableLimit` argument regardless of nesting ([Calcite source](https://github.com/apache/calcite/blob/calcite-1.40.0/core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableLimit.java#L111-L118)). Supported SQL Federation subquery paths can consequently still fail with the original `Long cannot be cast to Integer` error. This leaves the linked issue’s requirement to identify pagination-owned indexes incomplete. > * **Required Change:** Please derive pagination parameter indexes from the complete SELECT tree used by the compiler, or propagate equivalent compiler-owned metadata to the binding boundary. Add a regression that exercises a `Long` pagination marker inside a nested or combined SELECT without supplying the index through a mocked root `PaginationContext`. > > ### Review Details > * **Review Focus:** Code Correctness Review; CI not reviewed by request. > * **Reviewed Scope:** All three files in GitHub `/pulls/39237/files`: `kernel/sql-federation/core` production and unit-test changes, plus the MySQL SQL Federation E2E case. Latest head: `f6b60b9da539c0d31ab021ab1178223089a5b273`; base and merge-base: `5486b2be95f7c9f77f77abe0036f2c77fbdb825e`. An isolated merge-base-to-head content comparison matched the authoritative three-file GitHub list. > * **Not Reviewed Scope:** GitHub Actions/check-runs and live MySQL/Docker E2E execution. Unrelated local working-tree changes were excluded. > * **Verification:** > > * `./mvnw -pl kernel/sql-federation/core -DskipITs -Dspotless.skip=true -Dtest=org.apache.shardingsphere.sqlfederation.engine.processor.impl.StandardSQLFederationProcessorTest -Dsurefire.failIfNoSpecifiedTests=false test` — exit 0; 15 tests passed. > * Focused `SelectStatementConverterTest`, `SubqueryTableConverterTest`, and `PaginationValueSQLConverterTest` — exit 0; 7 tests passed. > * `./mvnw -pl kernel/sql-federation/core -DskipTests spotless:check checkstyle:check -Pcheck -T1C` — exit 0; Spotless clean and zero Checkstyle violations. > * Calcite behavior was verified against the repository-declared version, 1.40.0. > * **Release Note / User Docs:** Not required; this is an internal behavior fix with no configuration, API/SPI, syntax, or migration change. Thanks for the review. I have addressed the P1 issue in commit 5e73a4c. The parameter binding now collects pagination parameter indexes from the full `SelectStatementContext` tree, including nested subquery contexts, instead of only using the root `SelectStatementContext#getPaginationContext()`. I also updated the tests so the pagination indexes are derived from real `SelectStatementContext` instances, and added a regression case for a derived-table subquery where the root SELECT has no `LIMIT`, but the nested SELECT has `LIMIT ?` bound with a `Long` parameter. The test verifies that only the nested pagination parameter is converted to `Integer`, while non-pagination `Long` parameters remain unchanged. Local validation completed: - `.\mvnw.cmd spotless:apply -Pcheck -T1C` - `.\mvnw.cmd checkstyle:check -Pcheck -T1C` - `.\mvnw.cmd -pl kernel/sql-federation/core -DskipITs -Dspotless.skip=true -Dtest=org.apache.shardingsphere.sqlfederation.engine.processor.impl.StandardSQLFederationProcessorTest -DfailIfNoTests=true -Dsurefire.failIfNoSpecifiedTests=false test` Please re-review when convenient. -- 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]
