Summary

• Merge Decision: Not Mergeable
• Reason: The patch filters generated values only after execution, while explicit auto-increment inserts can still request and read backend generated keys before that filter.

Positive Feedback

• Using GeneratedKeyContext.isGenerated() is the right direction for separating generated values from client-supplied values, and the PR includes a release-note entry.

Issues

• P1 Root-cause path still requests backend generated keys for explicit values (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:189)
  • Problem: Normal Proxy execution still sets isReturnGeneratedKeys for every InsertStatement when the dialect has a generated-key option, without checking InsertStatementContext.getGeneratedKeyContext().isGenerated(). That flag is passed into StatementOption and can still reach statement.getGeneratedKeys() / resultSet.getLong(1) at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:75 and proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:86. The same unconditional flag exists on the federation path at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:264. The new .filter(GeneratedKeyContext::isGenerated) at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:334 is too late to prevent the linked issue's backend generated-key read failure, and it also cannot suppress a positive explicit value already carried in UpdateResult.lastInsertId, which UpdateResponseHeader keeps at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/response/header/update/UpdateResponseHeader.java:67.
  • Impact: The reported Proxy/MySQL rule-managed insert with explicit -3 can still fail before UpdateResponseHeader is built, and an adjacent explicit positive auto-increment value can still be returned as lastInsertId. This leaves issue #38717 only partially fixed.
  • Required Change: Please gate RETURN_GENERATED_KEYS and the generated-key callback path with the same explicit/generated distinction, for both normal and federation execution paths if both are reachable. Please also add regression coverage that does not mock away ProxyJDBCExecutorCallback: verify explicit auto-increment values use NO_GENERATED_KEYS or otherwise never call getGeneratedKeys(), and keep a generated-value case to prove real generated keys still work.

Review Details

• Reviewed Scope: Latest PR head 97cca573cb323987908b6620cd155acf0731c9fc; GitHub base SHA 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local triple-dot file list matched GitHub /pulls/38810/files. Reviewed RELEASE-NOTES.md, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java, proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java, and the related generated-key execution / OK-packet path. Shared-path blast radius checked for MySQL and MariaDB generated-key metadata; other dialect metadata defaults to no generated-key option.
• Not Reviewed Scope: CI/check statuses and full live Proxy + MySQL reproduction were not reviewed; no code changes were made.
• Verification: git fetch apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; git diff --name-status refs/remotes/apache/master...refs/remotes/apache/pr/38810 exit 0 and matched GitHub files; static git show / git grep inspection exit 0. Maven tests were not run because the code-path blocker is visible and the added unit test bypasses the failing callback path.
• Release Note / User Docs: Release note is present in RELEASE-NOTES.md:29; user docs are not required for this internal Proxy bug fix, but the release-note claim should be kept only after the root-cause fix is complete.

Summary

• Merge Decision: Not Mergeable
• Reason: The latest patch fixes the original explicit -3 path too broadly and now suppresses generated keys for MySQL AUTO_INCREMENT values that are still generated when the key column is explicitly present as NULL or 0.

Positive Feedback

• The latest head is in the right direction compared with the previous round: it now gates StatementOption, the normal Proxy callback path, the federation path, and the final UpdateResponseHeader value propagation instead of only filtering after execution.

Issues

• P1 Explicit NULL/0 auto-increment inserts lose generated-key handling (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:266, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:431)
  • Problem: The new gate returns generated keys only when GeneratedKeyContext.isGenerated() is true, but GeneratedKeyContextEngine sets generated=false whenever the auto-increment column is present in the INSERT list (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/keygen/engine/GeneratedKeyContextEngine.java:59 and infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/keygen/engine/GeneratedKeyContextEngine.java:103). That correctly covers explicit nonspecial values such as -3, but it also covers INSERT INTO t(id, ...) VALUES (NULL, ...) and 0 under normal MySQL mode, where MySQL still generates an AUTO_INCREMENT value. The MySQL 8.4 C API documentation distinguishes explicit nonspecial values from NULL/0 values that generate auto-increment IDs.
  • Impact: Valid Proxy MySQL/MariaDB inserts that intentionally request database-side generation with an explicit NULL or 0 key column will now use Statement.NO_GENERATED_KEYS / prepareStatement(sql) and ProxyJDBCExecutorCallback will skip getGeneratedKeys() at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallback.java:75. That regresses last_insert_id / generated-key response behavior for an adjacent valid input while fixing issue #38717.
  • Required Change: Please distinguish explicit nonspecial client-supplied values from explicit special values that still ask MySQL/MariaDB to generate the auto-increment value. Keep suppressing generated-key reads for values like -3, but preserve backend generated-key handling for NULL and 0 where the storage engine generates the value. Please add regression tests for both sides: explicit nonspecial values return no generated key, while explicit NULL/0 generated cases still return the generated key.

Multi-Round Comparison

• Previous blocker: The prior review requested moving the fix earlier than UpdateResponseHeader so Proxy would not request/read backend generated keys for explicit nonspecial values. This is partially fixed in the latest head by gating both normal and federation execution paths.
• New blocker: The new gate uses GeneratedKeyContext.isGenerated() as a broader “should ask backend for generated keys” signal, but that flag does not model MySQL’s explicit NULL/0 generation semantics. The adjacent generated-key case remains untested.

Review Details

• Reviewed Scope: Latest PR head d0d9c09c91a48602130176bebfb6ba4e373eac00; GitHub base SHA 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local triple-dot file list matched GitHub /pulls/38810/files. Reviewed RELEASE-NOTES.md, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java, proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java, and the three changed test files, plus related generated-key binder/callback/OK-packet paths. Shared-path blast radius checked for MySQL and MariaDB, which expose DialectGeneratedKeyOption; dialects such as PostgreSQL/openGauss/SQL92 default to no generated-key option on this path. Review baseline included CODE_OF_CONDUCT.md:8, CODE_OF_CONDUCT.md:11, CODE_OF_CONDUCT.md:17, and CODE_OF_CONDUCT.md:18.
• Not Reviewed Scope: CI/check statuses, full live Proxy + MySQL reproduction, and unrelated dirty local workspace files were not reviewed or used for conclusions.
• Verification: git fetch apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; local git diff --name-status 8162bdf1377764c7cf4a52edd32c35f4d0a2796c..refs/remotes/apache/pr/38810 matched GitHub /pulls/38810/files exit 0; static git show / git grep inspection exit 0; git grep computeIfAbsent on changed production files returned no matches. Maven was not run because the merge blocker is visible in the control flow and the current tests do not cover the adjacent NULL/0 generated-key case.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:29; user docs are not required for this internal Proxy bug fix, but the release-note claim should remain only after the regression above is fixed.

Summary

• Merge Decision: Not Mergeable
• Reason: The latest patch still misses the literal NULL/0 generated-key path and keeps dialect-specific generated-key semantics in a shared executor through brittle reflection.

Positive Feedback

• The fix is moving in the right direction by gating StatementOption before JDBC execution in both normal and federation paths, and the release note is present.

Issues

• P1 Literal NULL/0 auto-increment inserts still skip generated-key handling (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:326)

  • Problem: The MySQL/MariaDB special-value branch calls getLiteral by reflection, but the real parser segment exposes getLiterals() (parser/sql/statement/core/src/main/java/org/apache/shardingsphere/sql/parser/statement/core/segment/dml/expr/simple/LiteralExpressionSegment.java:34). That exception is swallowed by the outer ReflectiveOperationException fallback, so literal INSERT ... VALUES (NULL, ...) and VALUES (0, ...) return false and use NO_GENERATED_KEYS. The added test does not catch this because it uses a local stub LiteralExpressionSegment with a fake getLiteral getter (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutorTest.java:505).
  • Impact: The previous-round counterexample remains broken for literal special values: MySQL documents NULL and 0 as values that still generate AUTO_INCREMENT IDs, while explicit nonmagic values do not affect LAST_INSERT_ID() (https://dev.mysql.com/doc/c-api/8.4/en/getting-unique-id.html).
  • Required Change: Please use the production InsertValueContext / ExpressionSegment APIs, or an explicit dialect-generated-key policy, and add regression tests with real LiteralExpressionSegment / InsertValueContext objects for literal NULL, literal 0, and a nonmagic value such as -3.

• P2 Dialect-specific generated-key semantics leak into the shared Proxy executor (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:284)

  • Problem: proxy/backend/core now hard-codes "MySQL" / "MariaDB" and parser class simple names, then silently falls back to false on reflection failures. This shared execution path is used before both normal and federation JDBC execution, while the existing dialect hook is DialectDatabaseMetaData.getGeneratedKeyOption().
  • Impact: The implementation is fragile for a high-frequency DML path and already let a production API mismatch pass tests. It also puts target-dialect semantics into a shared module instead of a typed dialect contract.
  • Required Change: Please move the NULL/0 special-value decision behind a generic dialect generated-key contract or another typed owner, avoid getSimpleName / reflection in the shared executor, and keep a non-target dialect case proving the shared path still returns false.

Multi-Round Comparison

• Fixed from previous round: The generated-key decision is no longer only response-side filtering; it now reaches StatementOption and the callback path for normal and federation execution.
• Partially fixed from previous round: The explicit NULL/0 case is covered for parameter markers, but literal NULL/0 remains broken and the current test uses stubs instead of production parser segments.
• Newly introduced issue: The latest implementation adds hard-coded MySQL/MariaDB and parser-class-name checks to the shared Proxy executor.

Review Details

• Reviewed Scope: Latest PR head 32d068f729ff548f5d120ea64c68a2ffbfb39f95; GitHub PR base SHA and local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local triple-dot file list matched GitHub /pulls/38810/files. Reviewed the 6 changed files plus related generated-key classes and MySQL generated-key documentation.
• Not Reviewed Scope: CI/check status, full live Proxy + MySQL/MariaDB reproduction, and a full Maven build were not reviewed.
• Verification: git fetch apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; local triple-dot file list comparison against GitHub /pulls/38810/files exit 0 and matched; static inspection confirmed no production getLiteral() API and confirmed LiteralExpressionSegment#getLiterals(). Maven was not run because the blocker is visible in the latest code and the current tests mask it with stubs.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:29; user docs are not required for this internal Proxy generated-key bug fix, but the release note should remain only after the blockers above are fixed.

Summary

• Merge Decision: Not Mergeable
• Reason: The latest head fixes the earlier generated-key retrieval path, but the MySQL/MariaDB auto-increment trigger-value matrix is still incomplete because the valid DEFAULT path is not handled or tested.

Positive Feedback

• The latest revision is clearly moving in the right direction: generated-key retrieval is now gated before JDBC execution in both normal and federation paths.
• The previous shared-executor reflection / parser-class-name approach has been removed, and MySQL/MariaDB semantics are now modeled through a dialect generated-key option.
• The explicit negative-value symptom from #38717 is covered more directly, and the release note is present.

Issues

• P1 DEFAULT auto-increment values are not handled by the generated-key gate (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:316)
  • Problem: MySQL grammar accepts DEFAULT as an insert assignmentValue (parser/sql/engine/dialect/mysql/src/main/antlr4/imports/mysql/DMLStatement.g4:117), and the MySQL visitor maps non-expr assignment values to CommonExpressionSegment (parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java:1770). The new gate only delegates ParameterMarkerExpressionSegment and LiteralExpressionSegment to the dialect option, then returns false for all other expression segment types (ProxySQLExecutor.java:316). MySQL documents DEFAULT as valid INSERT syntax and documents generated auto-increment / LAST_INSERT_ID() behavior: https://dev.mysql.com/doc/refman/8.4/en/insert.html and https://dev.mysql.com/doc/refman/8.4/en/information-functions.html.
  • Impact: An adjacent valid input such as INSERT INTO t(id, name) VALUES (DEFAULT, 'a') can use NO_GENERATED_KEYS, skip getGeneratedKeys(), and return lastInsertId=0 even when the backend generates an AUTO_INCREMENT value. This also affects MariaDB through the delegated MySQL generated-key metadata path.
  • Required Change: Please complete the MySQL/MariaDB generated-key trigger-value matrix instead of only covering NULL and 0. DEFAULT should either be handled as a generated-key trigger, or the PR should provide official documentation and tests proving that ShardingSphere should intentionally not request generated keys for this syntax. Please add regression coverage using the real parser/binder expression shape, plus focused dialect-option tests for explicit non-magic values, NULL, 0, and DEFAULT.

Multi-Round Comparison

• Fixed from previous rounds: The latest head now gates generated-key retrieval before execution, preserves explicit NULL / 0 handling for literal and parameter-marker paths, and moves MySQL/MariaDB generated-key semantics into a dialect option instead of shared-executor reflection.
• Newly identified, not newly introduced: The DEFAULT auto-increment case is a remaining gap in the same generated-key trigger matrix. It was not introduced by the latest commits; earlier revisions also did not explicitly and reliably cover this parser/binder shape.
• Still blocking in latest head: The latest tests cover explicit non-magic values plus NULL / 0, but they still miss the valid MySQL/MariaDB DEFAULT branch.

Review Details

• Reviewed Scope: Latest PR head 31a60cebea24cd8c257418a08a9163449cc616a8; GitHub PR base SHA 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; fetched apache/master SHA 7a6cbfec2f3f185adef2f4a60408b0fd78d5a2b9; local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c. Local triple-dot file list matched GitHub /pulls/38810/files for all 9 changed files: RELEASE-NOTES.md, connector generated-key metadata, MySQL metadata option, Proxy executor / connector paths, and the three changed Proxy tests.
• Not Reviewed Scope: CI/check statuses, full live Proxy + MySQL/MariaDB smoke, and a full Maven build were not reviewed.
• Verification: git fetch --no-tags apache master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; GitHub /pulls/38810/files API comparison against local git diff --name-status 8162bdf1377764c7cf4a52edd32c35f4d0a2796c..apache/pr/38810 exit 0 and matched; review inventory script exit 0; static git show / git grep inspection exit 0. Maven was not run because the merge blocker is visible in the static control flow and missing counterexample coverage.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:29; user docs are not required for this internal Proxy generated-key bug fix, but the release note should remain only after the blocker above is fixed.

Summary

Review Result: Not Mergeable

Feedback Mode: Change Request

Reason: The latest patch handles DEFAULT when the auto-increment column is explicitly listed, but it still loses the same generated-key trigger path for valid no-column-list inserts such as INSERT INTO t VALUES (DEFAULT, ...) / VALUES (NULL, ...).

Issues

• P1 No-column-list INSERT trigger values still skip generated-key retrieval
  • Problem: GeneratedKeyContextEngine already resolves generated-key position differently for explicit-column and no-column-list inserts: when insertColumnNamesAndIndexes is empty, it uses visible schema columns to decide whether the generated key is present and to find its value index (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/segment/insert/keygen/engine/GeneratedKeyContextEngine.java:76, GeneratedKeyContextEngine.java:108). The new Proxy gate loses that resolved index and recomputes it with insertStatementContext.getInsertColumnNames().indexOf(columnName) (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:296). But getInsertColumnNames() only returns explicitly written INSERT columns (infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/InsertStatementBaseContext.java:145), while no-column-list inserts expose visible schema columns through getColumnNames() (InsertStatementBaseContext.java:79, infra/binder/core/src/main/java/org/apache/shardingsphere/infra/binder/context/statement/type/dml/InsertStatementContext.java:174). Therefore INSERT INTO t VALUES (DEFAULT, 'a') never reaches the dialect trigger-value check and still uses NO_GENERATED_KEYS.
  • Impact: MySQL accepts no-column-list INSERT ... VALUES (...) and supports generated auto-increment values for DEFAULT, NULL, and 0. For Proxy MySQL/MariaDB, this can still skip getGeneratedKeys() and return lastInsertId=0 even though the backend generated an AUTO_INCREMENT value. This leaves the same generated-key trigger matrix incomplete.
  • Required Change: Please reuse or expose the binder-resolved generated-key value index instead of recomputing it from explicit insert columns only. Please add regression coverage for no-column-list generated-key triggers, at least VALUES (DEFAULT, ...) and one of VALUES (NULL, ...) / VALUES (0, ...), while keeping the explicit non-magic value case such as -3 on the NO_GENERATED_KEYS path.

Multi-Round Comparison

• Fixed from the previous round: DEFAULT is now modeled in MySQLGeneratedKeyOption, and ProxySQLExecutor delegates CommonExpressionSegment text to the dialect option.
• Still blocking in latest head: The trigger-value matrix is incomplete because the latest tests only cover explicit-column inserts; they do not cover the no-column-list production path where getInsertColumnNames() is empty.

Review Details

• Reviewed Scope: Latest PR head c8854f2101e270961357e75a787db80ef49659e8; GitHub PR base SHA and local merge-base 8162bdf1377764c7cf4a52edd32c35f4d0a2796c; local file list matched GitHub /pulls/38810/files for all 10 changed files.
• Not Reviewed Scope: Full live Proxy + MySQL/MariaDB smoke and full Maven build were not run.
• Verification: GitHub PR/files/reviews/check-runs API inspection exit 0; git fetch --no-tags apache master pull/38810/head:refs/remotes/apache/pr/38810 exit 0; local triple-dot file-list comparison against GitHub /pulls/38810/files exit 0 and matched; review inventory script exit 0; static git show / git grep inspection exit 0. GitHub check-runs for head c8854f2101e270961357e75a787db80ef49659e8 showed 83 successful checks, but CI success does not cover the missing no-column-list branch. Maven was not run because the blocker is visible in static control flow and the current tests do not include this counterexample.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:29. User docs are not required for this internal Proxy generated-key bug fix, but the release note should remain only after the generated-key trigger matrix is complete.

Summary

Review Result: Not Mergeable

Feedback Mode: Change Request

Reason: The functional generated-key handling path is now largely aligned with the linked issue, but the latest patch changes the shared generated-key option design in a way that is inconsistent with the existing connector metadata option model. This shared API shape should be corrected before merge.

Issues

• P1 DialectGeneratedKeyOption should follow the existing interface + default implementation design
  • Problem: This PR changes DialectGeneratedKeyOption from a final value object into a non-final concrete superclass at database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/metadata/database/metadata/option/keygen/DialectGeneratedKeyOption.java:28, then makes MySQLGeneratedKeyOption extend it at database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/metadata/database/option/MySQLGeneratedKeyOption.java:25. This is inconsistent with the existing connector metadata option design. For behavior-bearing dialect options, the current system uses an interface plus a default implementation, for example DialectDataTypeOption + DefaultDataTypeOption, DialectFunctionOption + DefaultFunctionOption, and DialectSchemaOption + DefaultSchemaOption.
  • Impact: The generated-key option now exposes a shared concrete inheritance contract that does not match the surrounding metadata option model. Future dialect-specific generated-key behavior would be encouraged to subclass a concrete base class instead of implementing a dialect option contract, making the shared connector metadata API less consistent and harder to maintain.
  • Required Change: Please make DialectGeneratedKeyOption an interface, add DefaultGeneratedKeyOption as the default implementation for the existing column-name / non-trigger behavior, and change MySQLGeneratedKeyOption to implement DialectGeneratedKeyOption directly. The MySQL/MariaDB metadata path can continue returning new MySQLGeneratedKeyOption(), while any dialect that only needs the previous behavior can return new DefaultGeneratedKeyOption("GENERATED_KEY").

Review Details

• Review Focus: Code Correctness Review. CI not reviewed by request.
• Reviewed Scope: Latest reviewed PR head 35897fff32507ae48e3d75827f48f39be54c225d; base ref master at PR API base b6ff7ce98d22f2afc3691a766c01f01f8717310d; local merge-base b6ff7ce98d22f2afc3691a766c01f01f8717310d. GitHub /pulls/38810/files matched the local triple-dot file list. Reviewed all 10 changed files: release note, connector generated-key option/metadata/test files, and Proxy executor/connector/callback production and test files.
• Not Reviewed Scope: GitHub Actions/check-runs/logs, full repository build, full Proxy E2E/native-client smoke, and unrelated modules outside the generated-key handling path.
• Verification: Focused unit tests passed locally on the reviewed PR head with exit code 0: ./mvnw -pl database/connector/core,database/connector/dialect/mysql,proxy/backend/core -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dtest=org.apache.shardingsphere.database.connector.mysql.metadata.database.option.MySQLGeneratedKeyOptionTest,org.apache.shardingsphere.proxy.backend.connector.ProxySQLExecutorTest,org.apache.shardingsphere.proxy.backend.connector.StandardDatabaseProxyConnectorTest,org.apache.shardingsphere.proxy.backend.connector.jdbc.executor.callback.ProxyJDBCExecutorCallbackTest -Dsurefire.failIfNoSpecifiedTests=false test. Summary: Tests run: 80, Failures: 0, Errors: 0, Skipped: 0; BUILD SUCCESS.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:31. Additional user docs are not required because this is a Proxy bug fix with no new configuration, migration step, or user-facing syntax change.

Summary

Review Result: Not Mergeable

Feedback Mode: Change Request

Reason: The PR is moving in the right direction for explicit auto-increment values, but two P1 issues still need to be addressed: the generated-key option interface still owns fallback trigger semantics even though DefaultGeneratedKeyOption now exists, and the new explicit generated-key lookup is case-sensitive in a path that must be case-insensitive for MySQL/MariaDB column names.

Issues

• P1: Do not keep generated-key trigger policy as an interface default

  DialectGeneratedKeyOption now has a concrete fallback implementation, DefaultGeneratedKeyOption, so isGeneratedKeyTriggerValue(final Object value) should not keep a default false implementation in the interface.

  With the current shape, the interface still owns the default trigger-value policy, while DefaultGeneratedKeyOption only stores the generated key column name. This makes the default implementation incomplete and lets any future dialect implementation silently inherit false even when that dialect should make an explicit decision about trigger values.

  Please make isGeneratedKeyTriggerValue(...) a required method on DialectGeneratedKeyOption, move the return false behavior into DefaultGeneratedKeyOption, and keep the MySQL-specific NULL / 0 / DEFAULT behavior in MySQLGeneratedKeyOption. This keeps the interface as a pure contract and makes each implementation explicitly own its generated-key semantics.

• P1: Case-sensitive generated-key column lookup loses trigger values for mixed-case columns

  ProxySQLExecutor.isReturnGeneratedKeysForExplicit(...) locates the generated key column with insertStatementContext.getColumnNames().indexOf(columnName) at proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:296.

  For explicit insert columns, InsertStatementBaseContext lowercases SQL column names, while the generated key column name comes from schema metadata and can preserve the original casing. Existing generated-key binding already uses case-insensitive maps for this lookup, but the new helper rechecks the column position with a case-sensitive List#indexOf.

  For example, with a MySQL table like CREATE TABLE t (ID INT AUTO_INCREMENT PRIMARY KEY, name VARCHAR(10)), an insert such as INSERT INTO t (ID, name) VALUES (DEFAULT, 'a') can have explicit insert columns represented as ["id", "name"], while GeneratedKeyContext#getColumnName() remains "ID". The current lookup returns -1, so Proxy executes with NO_GENERATED_KEYS even though DEFAULT, NULL, and normally 0 should still trigger auto-increment key generation.

  Please make this generated-key column index lookup case-insensitive, or reuse the existing case-insensitive insert-column index mapping, and add a regression test where the schema generated column casing differs from the explicit insert column casing for a trigger value such as DEFAULT, NULL, or 0.

Multi-Round Comparison

The latest revision addresses the earlier shared-option design concern by introducing DefaultGeneratedKeyOption and making DialectGeneratedKeyOption an interface. However, the interface still retains fallback trigger-value behavior, so the abstraction boundary is not fully corrected yet.

The current revision also introduces the explicit generated-key decision path, but that path needs to preserve the existing case-insensitive column semantics.

Review Details

• Review Focus: Code correctness and interface design review. CI/check-runs were not reviewed.
• Reviewed Scope: latest PR head dfbbdc4fff3aa61c01c91a898df53f17fbda2c73; generated-key option API, MySQL generated-key option, Proxy generated-key execution path, response header behavior, and related tests.
• Reference behavior: MySQL treats omitted/NULL/normally 0 auto-increment values as generated values, while other explicit values are stored as supplied: https://dev.mysql.com/doc/refman/8.4/en/example-auto-increment.html
• Verification: Static review of the PR diff and adjacent generated-key binding/schema metadata paths. Maven and CI were not run for this review.

Summary

Review Result: Mergeable

Reason: This is a code-scope result only. CI was not reviewed by request. The latest head fixes the original explicit auto-increment generated-key path and addresses the prior shared-option design and case-insensitive lookup blockers.

Evidence

• The root path is now gated before JDBC execution. ProxySQLExecutor computes isReturnGeneratedKeys from InsertStatementContext, generated-key context, dialect metadata, and SQL parameters before constructing StatementOption, so explicit non-trigger values such as -3 use the non-generated-key path while generated cases still request keys (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutor.java:196, ProxySQLExecutor.java:276).
• The explicit-value trigger policy is owned by the dialect option. DialectGeneratedKeyOption is now a pure contract, DefaultGeneratedKeyOption owns the default non-trigger behavior, and MySQLGeneratedKeyOption handles MySQL trigger values NULL, 0, and DEFAULT while rejecting ordinary explicit values (database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/metadata/database/metadata/option/keygen/DialectGeneratedKeyOption.java:23, DefaultGeneratedKeyOption.java:36, database/connector/dialect/mysql/src/main/java/org/apache/shardingsphere/database/connector/mysql/metadata/database/option/MySQLGeneratedKeyOption.java:33). This matches MySQL documented AUTO_INCREMENT behavior for 0, NULL, and other explicit values, and MySQL’s LAST_INSERT_ID() semantics for automatically generated values: AUTO_INCREMENT, LAST_INSERT_ID().
• The earlier no-column-list and mixed-case risks are covered by the latest implementation. The generated-key column lookup iterates InsertStatementContext.getColumnNames() with equalsIgnoreCase, which preserves binder-resolved column ordering for both explicit-column and no-column-list inserts (ProxySQLExecutor.java:294).
• The response path no longer turns explicit auto-increment values collected by the binder into lastInsertId; StandardDatabaseProxyConnector only passes binder generated values when GeneratedKeyContext.isGenerated() is true (proxy/backend/core/src/main/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnector.java:329).
• Regression coverage now includes explicit non-trigger values, NULL, 0, DEFAULT, no-column-list inserts, mixed-case generated-key column names, federation delegation through the same gate, and callback behavior that avoids Statement#getGeneratedKeys() when generated keys are disabled (proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/ProxySQLExecutorTest.java:301, proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/StandardDatabaseProxyConnectorTest.java:833, proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/jdbc/executor/callback/ProxyJDBCExecutorCallbackTest.java:237).

Review Details

• Review Focus: Code Correctness Review. CI not reviewed by request.
• Reviewed Scope: Latest PR head aa622587b751cdc4fa5e299b123b690281d42b8a; PR API base ref master at b6ff7ce98d22f2afc3691a766c01f01f8717310d; local merge-base b6ff7ce98d22f2afc3691a766c01f01f8717310d. GitHub /pulls/38810/files matched the local triple-dot file list. Reviewed all 12 changed files: release note, connector generated-key option API/default/MySQL implementations and tests, Proxy executor/federation/update response path, and callback/regression tests.
• Not Reviewed Scope: GitHub Actions/check-runs/logs, full live Proxy + MySQL/MariaDB smoke, and unrelated modules outside the generated-key handling path.
• Verification: GitHub PR/files/comments/reviews public API fetch: exit 0; git fetch --no-tags apache master:refs/remotes/apache/master pull/38810/head:refs/remotes/apache/pr/38810: exit 0; local triple-dot file-list comparison against GitHub files: exit 0, matched. Focused PR-head unit tests: ./mvnw -pl database/connector/core,database/connector/dialect/mysql,proxy/backend/core -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dtest=org.apache.shardingsphere.database.connector.core.metadata.database.metadata.option.keygen.DefaultGeneratedKeyOptionTest,org.apache.shardingsphere.database.connector.mysql.metadata.database.option.MySQLGeneratedKeyOptionTest,org.apache.shardingsphere.proxy.backend.connector.ProxySQLExecutorTest,org.apache.shardingsphere.proxy.backend.connector.StandardDatabaseProxyConnectorTest,org.apache.shardingsphere.proxy.backend.connector.jdbc.executor.callback.ProxyJDBCExecutorCallbackTest -Dsurefire.failIfNoSpecifiedTests=false test: exit 0, Tests run: 81, Failures: 0, Errors: 0, Skipped: 0. Scoped style authority check: ./mvnw -pl database/connector/core,database/connector/dialect/mysql,proxy/backend/core -DskipTests spotless:check checkstyle:check -Pcheck -T1C: exit 0, BUILD SUCCESS.
• Release Note / User Docs: Release note is present at RELEASE-NOTES.md:31. Additional user docs are not required because this is a Proxy bug fix with no new configuration, migration step, or user-facing SQL syntax.