Implement basic BATCH in Firebird Proxy

[makssent]

Changes proposed in this pull request:

• Added basic functionality required for regular queries to work with BATCH.

---

Before committing this PR, I'm sure that I have checked the following options:

• My code follows the code of conduct of this project.
• I have self-reviewed the commit code.
• I have (or in comment I request) added corresponding labels for the pull request.
• I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
• I have made corresponding changes to the documentation.
• I have added corresponding unit tests for my changes.
• I have updated the Release Notes of the current development version. For more details, see Update Release Note

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: PR #38605 latest head edd7f017dfa507ba129024eb8ef23c908c46462f, merge-base e398f09e55e420caa393f7ae087a1bc1a313bcaf; local triple-dot file list matched GitHub /pulls/38605/files exactly, 45 files. Reviewed Firebird protocol packet/codec changes, Firebird Proxy batch executors/factories/tests, and the Firebird Wire Protocol batch and row-data sections.
• Not Reviewed Scope: I did not use CI/check-run status, and I did not run a full Firebird/Jaybird runtime smoke test. SQL parser and non-Firebird dialect modules were not in PR scope.
• Need Expert Review: Yes, a Firebird wire-protocol/proxy frontend reviewer should re-check after the blockers below are fixed.

Positive Feedback

• The direction is useful: the PR starts wiring op_batch_create, op_batch_msg, op_batch_exec, and op_batch_cancel into the Firebird protocol and proxy frontend, and adds unit coverage around several new packet and executor classes.

Major Issues

• [Blocking] Standard batch release is not implemented (database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/packet/command/FirebirdCommandPacketFactory.java:106)

  • Symptom: The PR handles create/message/execute/cancel, but BATCH_RLS remains only an enum value and falls through to unsupported packet/executor handling. The official Firebird wire protocol documents op_batch_rls (102) as a client batch command with a generic response: https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_release_batch
  • Risk: A normal client that releases the batch after execution can still fail against Proxy, and the registered batch state can remain until connection close or another create/cancel path. That means the root cause is only partially repaired.
  • Action: Please add packet length parsing, packet creation, executor handling, registry cleanup, and direct tests for op_batch_rls. If release is intentionally unsupported, please provide protocol/client evidence and add an explicit compatibility test for that behavior.

• [Blocking] Non-batch BLOB execution was regressed and appears unrelated to BATCH (database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/packet/command/query/statement/execute/FirebirdExecuteStatementPacket.java:83)

  • Symptom: Regular op_execute BLOB parameters no longer read the 8-byte blob id from the execute payload; they instead read from FirebirdBlobRegistry.buildSegmentPayload(...), and the updated test now expects null for a BLOB parameter.
  • Risk: This can break ordinary prepared statements with BLOB parameters and can also leave the packet reader index at the wrong position for the remaining execute fields. Firebird row data documents blr_quad / blr_blob2 values as 2x Int32, Int64, or 8 bytes, with the value being the blob id: https://www.firebirdsql.org/file/documentation/html/en/firebirddocs/wireprotocol/firebird-wire-protocol.html#_row_data_column_value
  • Action: Please roll back this non-batch execute-path change, or split it into a separately justified fix with protocol evidence and regression tests. Batch BLOB support should be implemented through the batch-specific commands such as op_batch_regblob / op_batch_blob_stream, not by changing regular execute semantics.

• [Blocking] Core batch execution and chunked batch decoding are not proven by production-path tests (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/batch/FirebirdBatchedStatementsExecutor.java:70)

  • Symptom: FirebirdBatchExecuteCommandExecutorTest mocks construction of FirebirdBatchedStatementsExecutor, so the new routing, SQL bind, prepare, PreparedStatement.addBatch(), and executeBatch() path is not directly exercised. FirebirdPacketCodecEngineTest also does not cover full or partial BATCH_MSG decoding.
  • Risk: The reported goal is “regular queries to work with BATCH”, but the tests mostly validate factory wiring and mocked executors. Counterexamples such as partial op_batch_msg, multi-message batches, release-after-execute, and missing/invalid batch state remain unvalidated.
  • Action: Please add direct tests for FirebirdBatchedStatementsExecutor and codec-level tests for full and chunked op_batch_msg input. Cover at least one adjacent/negative path such as unknown batch statement, release after execute, and multiple message rows.

• [Blocking] The codec still contains a temporary implementation marker (database/protocol/dialect/firebird/src/main/java/org/apache/shardingsphere/database/protocol/firebird/codec/FirebirdPacketCodecEngine.java:122)

  • Symptom: The BATCH message framing path is explicitly marked as a temporary TODO.
  • Risk: This is on the Firebird Proxy request decoding path, so a temporary implementation plus missing chunked-message tests leaves compatibility and packet-boundary behavior unclear.
  • Action: Please replace the temporary framing logic with the final approach and cover it with codec tests before merge.

Unrelated Changes

• Please roll back or split the regular FirebirdExecuteStatementPacket BLOB parameter behavior change. It is not part of the BATCH command path and changes existing non-batch execute semantics.

Next Steps

• Implement op_batch_rls support and registry cleanup with direct packet/factory/executor tests.
• Restore regular op_execute BLOB id parsing, or move any BLOB batch work to batch-specific commands with separate evidence.
• Add production-path tests for FirebirdBatchedStatementsExecutor and codec tests for complete and partial op_batch_msg frames.
• Remove the temporary codec TODO and rerun the relevant module tests plus Spotless/Checkstyle before requesting another review.

[terrymanu]

Summary

• Merge Decision: Not Mergeable
• Reason: Batch execution still returns completion-state update counters with physical execution-unit semantics instead of Firebird batch-message semantics.
• Expert Review Needed: A Firebird wire-protocol / Proxy frontend reviewer should re-check completion-state behavior after the update-count mapping is fixed.

Positive Feedback

• The latest head moves in the right direction: op_batch_rls is now wired through packet/factory/executor tests, the previous regular op_execute BLOB id parsing regression is restored, and there are new direct executor plus split op_batch_msg codec tests.

Issues

• P1 Batch completion update counts are emitted with the wrong semantic owner (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/batch/FirebirdBatchExecuteCommandExecutor.java:57)
  • Problem: FirebirdBatchCreateCommandExecutor parses TAG_RECORD_COUNTS, but registerBatch(...) stores only statement id, column types, and buffer size (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/batch/FirebirdBatchCreateCommandExecutor.java:97). Later FirebirdBatchedStatementsExecutor concatenates the int[] returned by each physical JDBC execution group (proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/batch/FirebirdBatchedStatementsExecutor.java:185), and FirebirdBatchExecuteCommandExecutor always writes that concatenated array into FirebirdBatchCompletionStateResponse. Firebird documents batch completion counters as records updated per batch message, and Jaybird documents the update-count array as empty when TAG_RECORD_COUNTS is not requested.
  • Impact: With TAG_RECORD_COUNTS omitted or false, Proxy still sends update counters. With a ShardingSphere multi-route path such as one Firebird batch message hitting a broadcast table or multiple sharding execution units, Proxy can send one counter per physical execution unit instead of one counter per Firebird batch message. That makes the protocol response incompatible with client expectations and leaves an adjacent-feature regression path unvalidated.
  • Required Change: Please preserve the parsed record-count request in the batch state, only emit per-message update counters when requested, and map or aggregate physical JDBC results back to the original Firebird batch-message indexes before writing op_batch_cs. Please add tests for the record-count-off path and a multi-route or broadcast counterexample in addition to the current single-storage happy path.

Multi-Round Comparison

• Fixed: The previous op_batch_rls blocker is addressed by the new release packet, factory, executor, and tests.
• Fixed: The previous non-batch BLOB execute regression is no longer present; regular FirebirdExecuteStatementPacket reads BLOB ids from the execute payload again.
• Partially fixed: The production-path testing gap improved with direct FirebirdBatchedStatementsExecutorTest and split op_batch_msg codec tests, but the latest tests still cover only a single-storage happy path and do not validate record-count-off or multi-route completion-counter behavior.
• Current blocker: The latest completion-state implementation exposes physical JDBC execution counts as Firebird batch-message counts and ignores the parsed TAG_RECORD_COUNTS flag.

Review Details

• Reviewed Scope: PR #38605 latest head e6025e9a50793a6ad697aea154e3cb2a5031b331, with local merge-base 44abc8bec0809a734c2d447b45429682c6ffa6f0. The local triple-dot file list matched GitHub /pulls/38605/files exactly: 51 Firebird protocol/proxy files.
• Reviewed Areas: Firebird batch packet decoding, command packet factory routing, batch registry/state, completion-state response encoding, proxy batch executors, frontend lifecycle cleanup, new tests, prior public review comments, and Firebird/Jaybird batch completion semantics.
• Not Reviewed Scope: I did not inspect GitHub Actions or CI results. I did not run a real Firebird/Jaybird runtime smoke. Non-Firebird dialect modules were considered only as adjacent ShardingSphere routing/broadcast/sharding execution behavior, not line-reviewed.
• Verification:
  • ./mvnw -pl database/protocol/dialect/firebird,proxy/frontend/dialect/firebird -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dlicense.skip=true -Dtest=FirebirdPacketCodecEngineTest,FirebirdBatchCreateCommandExecutorTest,FirebirdBatchExecuteCommandExecutorTest,FirebirdBatchedStatementsExecutorTest,FirebirdBatchCompletionStateResponseTest -Dsurefire.failIfNoSpecifiedTests=false test -> exit 0
  • ./mvnw -pl database/protocol/dialect/firebird,proxy/frontend/dialect/firebird -am -DskipTests checkstyle:check -Pcheck -T1C -> exit 0