strongduanmu commented on PR #38706:
URL: https://github.com/apache/shardingsphere/pull/38706#issuecomment-4497501185
• Merge Verdict: Mergeable
Reviewed Scope
- PR latest head: 1aa922450ede2ab880ae4a34cb01bea0b22c0181
- Changed file:
jdbc/src/main/java/org/apache/shardingsphere/driver/executor/engine/pushdown/jdbc/DriverJDBCPushDownExecuteExecutor.java
- Adjacent/reference paths: JDBC executeUpdate executor,
DriverExecuteExecutor, DatabaseTypeRegistry,
ShardingSphereConnection#getAutoCommit, PushDownMetaDataRefreshEngine,
TransactionRule, and the analogous Proxy implicit-commit path.
Not Reviewed Scope
- GitHub CI/check status, per your request.
- Full Maven build was not rerun in this review; your local CI/e2e pass
statement was used as validation background.
Need Expert Review
- No.
Basis
- This change only moves
DatabaseTypeRegistry(...).getDialectDatabaseMetaData().getTransactionOption()
to the tail of the && short-circuit chain, and removes the single-use
DialectTransactionOption local variable plus import.
- The boolean result remains equivalent: autoCommit=true or
non-DDLStatement returns false; autoCommit=false && DDLStatement is still
decided by dialect isDDLNeedImplicitCommit().
- DatabaseTypeRegistry construction only reads SPI metadata and does not
mutate business state; deferring it is a performance short-circuit optimization
with no fallback logic, swallowed exception, or commit-order change.
- The commit point remains after jdbcExecutor.execute(...) and before
pushdown metadata refresh, so DDL implicit-commit semantics are preserved.
- Risk scan found no API/SPI, configuration, SQL parser, routing, default
schema, name resolution, or dependency change.
- High-frequency path rule passed: the changed file has no
ConcurrentHashMap#computeIfAbsent.
- Adversarial scenarios checked: non-DDL Statement.execute no longer
triggers dialect metadata lookup; DDL on dialects such as Firebird where
DDLNeedImplicitCommit=true still checks the option and commits when
autoCommit=false; MySQL/PostgreSQL paths with the
option set to false still do not commit.
Pre-merge Checks
- git diff --check
refs/remotes/upstream/master...refs/remotes/upstream/pr/38706: exit 0
- git diff --name-status
refs/remotes/upstream/master...refs/remotes/upstream/pr/38706: exit 0, only one
JDBC file changed
- git grep computeIfAbsent ... DriverJDBCPushDownExecuteExecutor.java:
exit 0, no matches
- No code was modified during this review; the working tree stayed clean.
Next step: proceed with merge; before merging, clean up the PR description
typo Fixes #ISSUSE_ID.
--
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]
