Support for executing DistSQL for ShardingSphere JDBC logical databases#37778
Support for executing DistSQL for ShardingSphere JDBC logical databases#37778linghengqian wants to merge 2 commits into
Conversation
6b8d6bd to
dd59ea6
Compare
b6d0b2c to
6f6ed14
Compare
6f6ed14 to
ae4f04b
Compare
c6bf6bf to
9d63ecd
Compare
9d63ecd to
761dcd5
Compare
terrymanu
left a comment
There was a problem hiding this comment.
Good direction: wiring DistSQL into JDBC with a dedicated executor/result set and adding JDBC-side DistSQL docs is the right direction—please keep pushing on this path.
Change requests:
- Most DistSQL executors live under proxy/backend/handler/distsql, and the JDBC module only depends on infra-distsql-handler. In JDBC the majority of DistSQL statements will not find executors on the classpath and will fail. Please either provide JDBC-usable executors for the intended statements or clearly scope the supported DistSQL set (with tests/docs).
- DriverDistSQLExecutor#createDistSQLConnectionContext builds a context with JDBC’s DatabaseConnectionManager and a null executorStatementManager, while executors like proxy/backend/core/.../PreviewExecutor.java:137 expect a ProxyDatabaseConnectionManager and a non-null statement manager. In JDBC this will lead to ClassCastException/NullPointerException. Please supply a JDBC-compatible context or adjust executors to be JDBC-safe.
- ShardingSphereStatement caches metaData at construction; after DistSQL updates, the same statement continues to use stale metadata, so newly registered storage units/rules are invisible to subsequent SQL. Please refresh metadata per execution or otherwise sync with MetaDataContexts.
- test/native/.../ShardingTest now depends on DistSQL dynamic registration; given the above gaps, this path will fail. Consider keeping the YAML fallback or delay the test switch until DistSQL-on-JDBC is functional.
Please address the above and add integration coverage for “DistSQL register/create → regular DML” in JDBC once the fixes are in. Keep going—this feature will be valuable once the JDBC path is complete.
linghengqian
left a comment
There was a problem hiding this comment.
DriverDistSQLExecutor#createDistSQLConnectionContext builds a context with JDBC’s DatabaseConnectionManager and a null executorStatementManager, while executors like proxy/backend/core/.../PreviewExecutor.java:137 expect a ProxyDatabaseConnectionManager and a non-null statement manager. In JDBC this will lead to ClassCastException/NullPointerException. Please supply a JDBC-compatible context or adjust executors to be JDBC-safe.
I don't see any unit tests throwing ClassCastException locally or in CI. Where is this error coming from?
test/native/.../ShardingTest now depends on DistSQL dynamic registration; given the above gaps, this path will fail. Consider keeping the YAML fallback or delay the test switch until DistSQL-on-JDBC is functional.
ShardingTest did not fail. Where are the failure logs? The tests containing this one, https://github.com/apache/shardingsphere/actions/runs/21162151539/job/60861111930 and https://github.com/apache/shardingsphere/actions/runs/21162151539/job/60861112341, both succeeded. Dozens of other unit tests using YAML in the same directory as this unit test are still working correctly.
This is core issue |
linghengqian
left a comment
There was a problem hiding this comment.
Most DistSQL executors live under proxy/backend/handler/distsql, and the JDBC module only depends on infra-distsql-handler.
This is core issue
My idea is that there shouldn't be any DistSQL instances that ShardingSphere JDBC can't use because the handling of the ContextManager is shared. If executing DistSQL previously threw an exception in JDBC, it will still throw the same exception if a Maven module becomes unreachable.
It's difficult to test the specific list of available DistSQL instances within JDBC class-level unit tests because ShardingSphere's existing unit tests don't involve the actual database. Once testing the actual database is required, a large number of integration tests containing DistSQL instances would need to be added to test-e2e or test-native, which doesn't seem suitable for the current PR.
|
I accept this requirement, JDBC should be able to use DistSQL just like Proxy. However, the current situation is that a large portion of DistSQL implementations reside in the Proxy module, so simply enabling JDBC calls won’t achieve that. It would first require moving the DistSQL-related logic from Proxy up to the kernel module. |
linghengqian
left a comment
There was a problem hiding this comment.
It would first require moving the DistSQL-related logic from Proxy up to the kernel module.
So, you mean to first merge #37867 , and then make JDBC Core depend on org.apache.shardingsphere:shardingsphere-distsql-handler in this PR, right?
|
This feature is too complex. |
linghengqian
left a comment
There was a problem hiding this comment.
-
I've changed the description of #37761 and the current PR; the current PR will no longer directly close the original issue. The original issue now has a task list.
- The distsql executor for RAL and RUL statements, which are independent of the ShardingSphere proxy, will be migrated into a kernel submodule.
- Modify
DriverDatabaseConnectionManagerandShardingSphereStatementto support executing distsql on the JDBC Driver. - Add E2E for DistSQL and ShardingSphere JDBC.
- Refactor the
shardingsphere-test-nativemodule to avoid most of the YAML file. - Add an infra URL impl like
jdbc:shardingsphere:scratch:sharding_dbto avoid creating the YAML file. - Refactor the
shardingsphere-test-nativemodule to avoid all YAML file. - Add document for DistSQL and ShardingSphere JDBC.
-
Does this mean that in the current PR, I only need to delete the newly added document and wait for another PR to merge it?
761dcd5 to
1ee8afb
Compare
linghengqian
left a comment
There was a problem hiding this comment.
@terrymanu Any updated reviews?
42ed955 to
5998e09
Compare
terrymanu
left a comment
There was a problem hiding this comment.
Decision
- Merge Verdict: Not Mergeable
- Reviewed Scope: PR #37778 latest head
5998e0981e2b836e7f25f234544249ac100a4a80;jdbcDistSQL executor/result set/statement/connection/pom/tests,test/nativeDistSQL test/yaml, JDBC DistSQL docs, and referencedinfra/distsql-handler/proxy/backend/coreDistSQL execution paths. - Not Reviewed Scope: Full Maven build, Spotless/Checkstyle, native-image execution, full DistSQL executor matrix, external database runtime validation, and GitHub Actions/CI.
- Need Expert Review: JDBC + DistSQL maintainer review is still needed for supported statement scope and executor ownership.
Positive Feedback
- Wiring DistSQL through JDBC
Statementand adding dedicated JDBC-side docs/tests is a useful direction.
Major Issues
-
[Blocker] The supported DistSQL scope is still overclaimed (
jdbc/pom.xml:77)- Symptom: JDBC only adds
shardingsphere-infra-distsql-handler, while the docs execute sharding and broadcast rule DistSQL (docs/document/content/user-manual/shardingsphere-jdbc/distsql/_index.en.md:85). Those executors live in feature-specific handler modules loaded through SPI, for examplefeatures/sharding/distsql/handler/.../DatabaseRuleDefinitionExecutor;DistSQLUpdateExecuteEnginedepends on those SPI instances atinfra/distsql-handler/src/main/java/org/apache/shardingsphere/distsql/handler/engine/update/DistSQLUpdateExecuteEngine.java:83. - Risk: A normal JDBC application following the docs can parse/dispatch into missing handlers or unsupported statements, while
test/nativecan mask this by having broader proxy/test dependencies. - Action: Please explicitly define the JDBC-supported DistSQL set, move or depend on the required JDBC-safe handler/parser modules, and align docs/tests with that exact scope.
- Symptom: JDBC only adds
-
[Blocker] The JDBC DistSQL context is not safe for proxy-only executors (
jdbc/src/main/java/org/apache/shardingsphere/driver/executor/engine/distsql/DriverDistSQLExecutor.java:101)- Symptom: The new context passes a JDBC
DatabaseConnectionManagerandnullexecutor statement manager, but proxy executors such asPreviewExecutorcast toProxyDatabaseConnectionManagerand use the statement manager (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/handler/distsql/rul/PreviewExecutor.java:137,proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/handler/distsql/rul/PreviewExecutor.java:160). - Risk: If proxy DistSQL executors are present on the classpath, JDBC DistSQL can fail with
ClassCastExceptionorNullPointerException; if they are absent, the feature is only partially available. - Action: Please either exclude/reject proxy-only DistSQL statements before SPI dispatch, or provide a JDBC-compatible context and executor path with negative coverage for proxy-only RUL statements.
- Symptom: The new context passes a JDBC
-
[Blocker] Same-statement SQL after DistSQL still uses stale metadata (
jdbc/src/main/java/org/apache/shardingsphere/driver/jdbc/core/statement/ShardingSphereStatement.java:107)- Symptom:
ShardingSphereStatementcapturesmetaDataanddriverExecutorFacadeat construction (ShardingSphereStatement.java:107,ShardingSphereStatement.java:111), then regular SQL execution continues with that cached metadata (ShardingSphereStatement.java:129). The docs run DDL/DML on the sameStatementimmediately after DistSQL updates (docs/document/content/user-manual/shardingsphere-jdbc/distsql/_index.en.md:89). - Risk: Newly registered storage units and newly created rules may not be visible to subsequent regular SQL on the same statement, so the documented
register/create -> DMLflow can regress. - Action: Please refresh metadata/facade after DistSQL updates, or otherwise enforce and document a new-statement boundary, with regression coverage for
REGISTER STORAGE UNIT -> CREATE RULE -> regular DMLon the same JDBC statement.
- Symptom:
-
[Major] Tests do not validate the production root path and one test violates the project test rule (
jdbc/src/test/java/org/apache/shardingsphere/driver/executor/engine/distsql/DriverDistSQLExecutorTest.java:87)- Symptom: The executor test reflectively invokes a private helper, while
CODE_OF_CONDUCT.md:96requires tests to exercise public APIs and forbids private-member reflection. The native test covers only a happy path (test/native/src/test/java/org/apache/shardingsphere/test/natived/jdbc/features/DistSqlTest.java:61) and does not cover missing handler modules, unsupported proxy-only statements, or same-statement post-DistSQL DML. - Risk: The tests can pass while the JDBC production path remains incomplete.
- Action: Please replace the private-reflection test with public
execute/executeQuery/executeUpdatecoverage and add integration cases mapped one-to-one to the supported JDBC DistSQL scenarios and counterexamples.
- Symptom: The executor test reflectively invokes a private helper, while
Next Steps
- Define the JDBC-supported DistSQL statement matrix and required Maven modules.
- Make unsupported/proxy-only statements fail clearly before SPI execution.
- Fix metadata refresh for post-DistSQL regular SQL.
- Add tests for same-statement DML after DistSQL, missing/unsupported executor paths, and JDBC-safe query/update statements.
- Update the English/Chinese docs to match the actual dependency and statement support.
- After fixes, run scoped verification such as
./mvnw -pl jdbc -am -DskipITs -Dspotless.skip=true -Dtest=DriverDistSQLExecutorTest,DistSQLResultSetTest,DistSQLResultSetMetaDataTest,DistSQLStatementContextTest -Dsurefire.failIfNoSpecifiedTests=false test, then Spotless and Checkstyle.
Multi-Round Comparison
- Previous feedback about most DistSQL executors not being JDBC-available: Not fixed; current JDBC dependency still only adds the infra handler and docs still show feature DistSQL.
- Previous feedback about proxy-specific context assumptions: Not fixed; the JDBC context still passes a JDBC manager and
nullstatement manager. - Previous feedback about stale metadata after DistSQL updates: Not fixed;
ShardingSphereStatementstill keeps constructor-time metadata/facade. - Previous feedback about native-test validation: Partially fixed; a dedicated
DistSqlTestwas added, but it does not cover the blocking counterexamples above.
Evidence Supplement
- Reviewer-run verification was static PR/code-path review only; Maven tests were not run because the blockers are visible from the latest diff.
- Additional evidence needed before merge: latest scoped module list, JDBC topology and mode support, target database versions, minimal reproducible inputs with expected/actual results, stack traces for unsupported paths, and one-to-one test evidence for every supported DistSQL fix point.
- SQL parser family review is not applicable in this round because the PR does not change grammar, visitor logic, or parser tests.
linghengqian
left a comment
There was a problem hiding this comment.
The current PR is blocked by #38682 because it requires an update to GRM. Let's address the GraalVM CE for JDK 25 issue first.
For #37761 .
Changes proposed in this pull request:
org.apache.shardingsphere.proxy.backend.distsql.DistSQLStatementContexttoorg.apache.shardingsphere.driver.jdbc.core.statement.DistSQLStatementContext.org.apache.shardingsphere:shardingsphere-test-nativemodule, nor does it discuss implementing support for JDBC URLs likejdbc:shardingsphere:scratch:logic_dbthat lack a YAML file style. This will be addressed through some separate issues.databaseNameandpropsproperties via DistSQL will not be addressed in the current PR.CREATE DATABASE sharding_dborUSE sharding_dbon ShardingSphere JDBC.org.apache.shardingsphere:shardingsphere-test-nativeare not yet separated into different modules, the documentation may be missing some Maven modules. I haven't had time to check[ERROR] Metadata copy process failed with code: 1is thrown when usingnative:metadata-copyin a multi-module Maven project graalvm/native-build-tools#650 and Native Build Tools 0.9.26'snative:metadata-copycausesMojoExecutionExceptionwhen executed on a multi-module Maven project graalvm/native-build-tools#500 yet, since I'm obviously not a graalvm committer.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.