nizhikov commented on PR #13614: URL: https://github.com/apache/ignite/pull/13614#issuecomment-5892910036
## Review summary: IGNITE-25090 binary writer/reader API trimming (`move1` vs `master`) **Scope.** 51 files, +683 / −1035. The branch removes implementation-only methods from `BinaryWriterEx` and `BinaryReaderEx`, drops the `forceHeap` stream variants and the three-argument `BinaryUtils.writer`, moves the JDBC/ODBC value encoding into the binary module (`writeJdbcObject`, `unmarshallJdbc`, `sqlTypeToBinary`, `jdbcTypeByClass`), and lets `BinaryObjectBuilderImpl` work directly with `BinaryWriterExImpl`. **Verdict: no behavioural regressions found.** Every removed API has zero remaining callers across `modules/` and `examples/`. The relocated JDBC/ODBC bodies are byte-for-byte equivalent to the code they replace. Findings below are design and cleanup items. ### Verification performed | Check | Result | |---|---| | Compile `ignite-binary-api`, `ignite-binary-impl`, `ignite-core` (main + test) | pass | | Test-compile `indexing`, `calcite`, `thin-client-impl`, `clients`, `control-utility` | pass | | Checkstyle on binary modules and core | pass | | Core: `BinaryMarshallerSelfTest`, 4 × `BinaryObjectBuilder*SelfTest`, `SqlListenerUtilsTest`, `RawBinaryObjectExtractorTest` | 734 tests, 0 failures | | Clients: `JdbcThinPreparedStatementSelfTest`, `JdbcThinResultSetSelfTest`, `JdbcBlobTest`, `JdbcBinaryBufferTest` | 88 tests, 0 failures | ### API surface change (interfaces) `BinaryWriterEx`: 25 methods removed (`preWrite`, `postWrite`, `postWriteHashCode`, `popSchema`, `writeFieldId`, `newWriter`, `schemaId(int)`, `array`, 8 × `write*FieldPrimitive`, `writeByteArray(InputStream,int)`, `writeBinaryObject`, `writeField`, `tryWriteAsHandle`, `writeBinaryArray`, `doWriteEnumArray`, `writeBinaryEnum`, `writeClass`, `writeProxy`); 1 added (`writeJdbcObject`). `BinaryReaderEx`: 6 removed (`unmarshal(int)`, `descriptor`, 2 × `unmarshalField`, `findFieldByName`, `getOrCreateSchema`); 1 added (`unmarshallJdbc`). ### Findings **Design decisions taken on purpose (documented so reviewers do not re-raise them)** 1. **JDBC/ODBC encoding lives in the binary module.** `BinaryWriterEx.writeJdbcObject` and `BinaryReaderEx.unmarshallJdbc` replace `SqlListenerUtils.writeObject` and `BinaryUtils.unmarshallJdbc`. Consequences: `BinaryWriterExImpl` imports `java.sql.Blob`, `sqlTypeToBinary`/`jdbcTypeByClass` sit in `BinaryUtils`, and `SqlInputStreamWrapper` moved to `ignite-binary-api` while keeping package `internal.processors.odbc`, so that package now spans two source modules. This was chosen over keeping nine writer primitives on the interface. Reading side note: `BinaryUtils.unmarshallJdbc` was already in binary-api on master; only the writer side is newly moved. 2. **`writeJdbcObject` and `writePlainObject` are two dispatch tables** in the same class (`BinaryWriterExImpl.java:1913` and `:2001`), with `SqlListenerUtils.isPlainType` as a third copy of the class list. They produce identical bytes for every class in `BinaryUtils.PLAIN_CLASS_TO_FLAG`; the only real differences are `java.sql.Date`, `java.sql.Date[]`, `SqlInputStreamWrapper`, `Blob` and the custom-object fallback. A map lookup delegating to `writePlainObject` would collapse the first table; kept as is. **Small cleanups worth doing** 3. **`GridBinaryMarshaller.java:255, :277`** and two tests (`BinaryMarshallerSelfTest:3045`, `RawBinaryObjectExtractorTest:52`) call `BinaryUtils.binariesFactory.writer(ctx, flag)` on the raw static field, while `BinaryUtils.writer(ctx, out)`, `writerWithoutSchema` and `reader(...)` still go through `BinaryUtils` wrappers. A two-argument `BinaryUtils.writer(BinaryContext, boolean)` wrapper keeps one construction path. Note: direct `binariesFactory` access already exists at ~30 other sites on this branch (calcite, indexing, thin-client, core), so this is consistency, not a new pattern. 4. **`CacheObjectBinaryProcessorImpl.java:264`**: `binaryMarsh = marsh.binaryMarshaller()` now aliases the node marshaller's internal instance instead of owning a `new GridBinaryMarshaller(binaryCtx)`. If `setBinaryContext` were ever called again on the shared marshaller, the processor would keep the stale instance. Single caller today, so latent; a one-line comment stating the single-instance intent is enough. 5. **`BinaryObjectBuilderDefaultMappersSelfTest.java:751-759`** (`testOffheapBinary`): the boolean + length prefix written into off-heap memory is dead setup left over from the removed `unmarshal(ptr, forceHeap)` format; the assertion now reads from `inputStream(ptr + 5, len)` directly. Allocate `arr.length`, copy at `ptr`, drop the prefix and the `+5`. Also restore the dropped `assertEquals(BinaryObjectOffheapImpl.class, offheapObj.getClass())` so a regression fails as a named assertion rather than a `ClassCastException`. 6. **`BinaryReaderEx.unmarshallJdbc(byte type, ...)`** keeps the master contract where the caller has consumed the type byte and the impl rewinds by one in the default branch (`BinaryReaderExImpl.java:2141`). Inherited from `BinaryUtils.unmarshallJdbc`, not new, but now it is an interface method the precondition is part of the public contract. Worth a javadoc sentence. **Pre-existing, moved verbatim, not introduced by this branch** 7. **`BinaryWriterExImpl.java:1979-1982`**: the Blob branch calls `blob.length()` three times and casts `long` to `int` before using it as the write limit. Identical on master in `SqlListenerUtils`. A Blob of 2 GiB or more yields a negative limit. If touched, read `long len = blob.length()` once and reject `len > MAX_ARRAY_SIZE`. 8. **Per-cell feature lookup**: `protoCtx.isFeatureSupported(CUSTOM_OBJECT)` is evaluated inside the row/argument loops in `JdbcUtils.writeItems`, `JdbcQuery` and `JdbcQueryExecuteRequest`. Same cost on master, hidden inside the old `JdbcUtils.writeObject`; hoisting one boolean per message is optional. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
