[PR] Implement write methods for PostgreSQLUnspecifiedBinaryProtocolValue [shardingsphere]

["daguimu (via GitHub)" <gi...@apache.org>]

daguimu opened a new pull request, #38570:
URL: https://github.com/apache/shardingsphere/pull/38570

   ## Problem
   
   `PostgreSQLUnspecifiedBinaryProtocolValue.write()` and `getColumnLength()` throw `UnsupportedSQLOperationException`, preventing unspecified-type values from being written through the PostgreSQL binary protocol path.
   
   ## Root Cause
   
   The `write()` and `getColumnLength()` methods were left as stubs throwing `UnsupportedSQLOperationException` when the class was initially created. This is one of the items tracked in #35830.
   
   ## Fix
   
   Implemented both methods following the same pattern as `PostgreSQLStringBinaryProtocolValue`:
   - `getColumnLength()`: returns the byte length of `value.toString()` using the payload's charset, correctly handling multi-byte characters
   - `write()`: writes the string representation via `payload.writeStringEOF(value.toString())`, which works for both plain String values and `PostgreSQLTypeUnspecifiedSQLParameter` instances (which delegate to their inner string via `toString()`)
   
   Removed the unused `UnsupportedSQLOperationException` import.
   
   ## Tests Added
   
   | Change Point | Test |
   |-------------|------|
   | `getColumnLength` returns correct byte length | `assertGetColumnLength` — `"val"` → 3, `PostgreSQLTypeUnspecifiedSQLParameter("test")` → 4 |
   | `getColumnLength` handles multi-byte characters | `assertGetColumnLengthWithMultiByteCharset` — `"中文"` → 6 (UTF-8) |
   | `write` outputs string bytes to payload | `assertWrite` — verifies `"val"` bytes written to ByteBuf |
   | `write` works with `PostgreSQLTypeUnspecifiedSQLParameter` | `assertWriteWithTypeUnspecifiedParameter` — verifies `"test"` bytes written |
   
   ## Impact
   
   Only affects the unspecified binary protocol value type in the PostgreSQL protocol module. Enables binary write support for unspecified-type columns. No changes to other protocol value implementations.
   
   Partial fix for #35830


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Implement write methods for PostgreSQLUnspecifiedBinaryProtocolValue [shardingsphere]

["RaigorJiang (via GitHub)" <gi...@apache.org>]

RaigorJiang merged PR #38570:
URL: https://github.com/apache/shardingsphere/pull/38570


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Implement write methods for PostgreSQLUnspecifiedBinaryProtocolValue [shardingsphere]

["RaigorJiang (via GitHub)" <gi...@apache.org>]

RaigorJiang commented on PR #38570:
URL: https://github.com/apache/shardingsphere/pull/38570#issuecomment-4531317116

   ### Decision
   
     **Merge Verdict: Mergeable**
   
     Reviewed Scope: latest PR head cb32ea2eec58eb7418aad39c90b2340d4a6c5f9f; database/protocol/dialect/postgresql only, specifically:
   
     - PostgreSQLUnspecifiedBinaryProtocolValue.java
     - PostgreSQLUnspecifiedBinaryProtocolValueTest.java
     - Related PostgreSQL protocol paths inspected: PostgreSQLDataRowPacket, PostgreSQLBinaryProtocolValueFactory, PostgreSQLStringBinaryProtocolValue, PostgreSQLPacketPayload,
       PostgreSQLBinaryColumnType, PostgreSQL proxy Portal, PostgreSQL/OpenGauss usage references for PostgreSQLBinaryColumnType.
   
     Not Reviewed Scope: full end-to-end client behavior with non-JDBC PostgreSQL drivers; unrelated PostgreSQL binary protocol value implementations still listed in issue #35830; full repository build was not run locally.
   
     Need Expert Review: No dedicated security/concurrency/performance expert review required. Protocol-domain review is useful but not blocking for this narrow change.
   
   ### Basis
   
     The PR directly fixes the root cause for its stated partial scope: PostgreSQLUnspecifiedBinaryProtocolValue#getColumnLength and write were stubs throwing UnsupportedSQLOperationException, so PostgreSQL binary result writing for UNSPECIFIED values failed at the protocol-value implementation point. The patch replaces those stubs with byte-length calculation using
     payload.getCharset() and writes the value bytes through payload.writeStringEOF(value.toString()).
   
     This matches the surrounding protocol design:
   
     - PostgreSQLDataRowPacket already writes the column length before delegating to PostgreSQLBinaryProtocolValue#write, so writeStringEOF correctly writes only value bytes and does not need a
       terminator.
     - PostgreSQL’s protocol documentation describes DataRow column data as an Int32 byte length followed by Byte n value bytes, where n is that length; no null terminator is part of the column
       value field. It also documents parameter/result format codes as text or binary, and parse parameter type OID 0 as unspecified. Source: PostgreSQL 17 protocol message formats, Bind,
       DataRow, and Parse sections: https://www.postgresql.org/docs/17/protocol-message-formats.html
     - The implementation follows the existing string binary protocol pattern, but keeps the scope limited to the UNSPECIFIED type.
   
   ### Test coverage is adequate for this partial fix:
   
     - getColumnLength is covered for a plain string, a PostgreSQLTypeUnspecifiedSQLParameter, and a multibyte UTF-8 case.
     - write is covered for a plain string and a PostgreSQLTypeUnspecifiedSQLParameter.
     - Existing read behavior remains covered.
     - CI reports success for required Checkstyle, Spotless, License, CI, and relevant PostgreSQL E2E smoke checks on the PR.
   
   ### Risk Assessment
   
     Design risk: Low. The change stays inside the PostgreSQL protocol binary value implementation and does not change factory registration, routing, metadata, parser behavior, SPI, or shared kernel paths.
   
     Performance risk: Low. The added work is a single toString() plus byte encoding, consistent with adjacent text/string handling. No ConcurrentHashMap#computeIfAbsent was introduced or changed in this high-frequency path.
   
     Compatibility risk: Low. The change enables a previously unsupported implementation for UNSPECIFIED; it does not change named type resolution, default schema, parser grammar, routing precedence, config semantics, or public API/SPI. Null handling is still owned by PostgreSQLDataRowPacket before the protocol value is called, so the null path remains unchanged.
   
     Cross-dialect/shared-path risk: Low. The touched code is in the PostgreSQL protocol module and can be referenced by PostgreSQL proxy paths and nearby OpenGauss tests that reuse PostgreSQL protocol types. The concrete non-target check is OpenGauss reference usage: it uses PostgreSQLBinaryColumnType.UNSPECIFIED in bind-related tests, but this PR changes only the binary value writer for PostgreSQL protocol value serialization and does not alter type parsing or parameter resolution. Adjacent binary types such as VARCHAR, CHAR, INT4, and null BinaryCell handling remain unchanged.
   
     Parser-family risk: Not applicable. No SQL grammar, visitor, lexer, parser test, or dialect parser behavior changed.
   
     Documentation risk: Low. This is an implementation completion for an existing protocol value type tracked by issue #35830; no user-facing syntax or configuration behavior changes were identified.
   
     Supply-chain risk: None. No dependency manifests or lockfiles changed.
   
   ### Pre-Merge Checks
   
     Commands/evidence used:
   
     - gh pr view 38570 --repo apache/shardingsphere ...
       Exit code: 0 after escalated GitHub API access. Confirmed latest head cb32ea2eec58eb7418aad39c90b2340d4a6c5f9f, two changed files, CI/check status, and PR description.
     - gh pr diff 38570 --repo apache/shardingsphere
       Exit code: 0 after escalated GitHub API access. Reviewed latest diff.
     - gh issue view 35830 --repo apache/shardingsphere ...
       Exit code: 0 after escalated GitHub API access. Confirmed this PR is a partial implementation of the PostgreSQLUnspecifiedBinaryProtocolValue item.
     - rg and sed inspections across PostgreSQL protocol/proxy/OpenGauss related paths
       Exit code: 0 for relevant inspections, except one rg readStringEOF query returned 1 because the exact searched read method does not exist locally.
     - Initial non-escalated gh calls failed with sandbox network denial, then were retried with approval as instructed.
   
   ### CI evidence from PR metadata:
   
     - Check - CheckStyle: success
     - Check - Spotless: success
     - Check - License: success
     - CI: success
     - PostgreSQL E2E SQL smoke checks for JDBC/Proxy and Standalone/Cluster: success
   
     Local Maven tests were not rerun because the PR CI already covers the required checkstyle/spotless gates and relevant PostgreSQL smoke checks, and the review did not require additional local Mockito/ByteBuddy execution. No sandbox-related Mockito attach blocker was encountered.


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Implement write methods for PostgreSQLUnspecifiedBinaryProtocolValue [shardingsphere]

["daguimu (via GitHub)" <gi...@apache.org>]

daguimu commented on PR #38570:
URL: https://github.com/apache/shardingsphere/pull/38570#issuecomment-4305530754

   Hi, I have just addressed the Spotless violations in a follow-up commit; CI should now pass. Would appreciate a review when someone has time. Thanks!


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org