[Improve][Connector-V2] Improve YashanDB vector type conversion#11397
[Improve][Connector-V2] Improve YashanDB vector type conversion#11397Tlinian wants to merge 1 commit into
Conversation
|
@DanielLeens @OctoberWithYou Hi! I have submitted this PR to add YashanDB vector type support. Could you please help review it when you are free? Thanks! |
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for this improvement! Upgrading the YashanDB JDBC driver and adding proper vector type support via the native Vector API is a solid direction. The read path with a Vector instanceof check plus a String fallback is a good defensive pattern.
I have a few observations:
Issue 1 (Medium): Missing E2E coverage for vector read/write paths
- The
JdbcYashanDbITE2E test was updated with the new driver URL, but it does not appear to include any vector column (FLOAT_VECTOR,FLOAT16_VECTOR,BFLOAT16_VECTOR) in its test schema. The core read/write logic for vectors was substantially rewritten, but the E2E test does not exercise the newVectorAPI path. Consider adding a vector column to the E2E test schema and verifying round-trip read/write.
Issue 2 (Low): FLOAT16_VECTOR / BFLOAT16_VECTOR read path uses toFloatArray() which loses precision distinction
- In
YashanDbJdbcRowConverter.toInternal(), theFLOAT16_VECTORandBFLOAT16_VECTORcases fall through to the same code asFLOAT_VECTOR, callingvector.toFloatArray()and wrapping asFloat[]. This is functionally fine if YashanDB only stores float32 vectors, but it means the 16-bit precision distinction is lost at read time. If this is intentional (YashanDB normalizes all vectors to float32 on read), a brief comment explaining that would help future readers.
Issue 3 (Low): trim() added to String fallback but not to original parse path
- Good catch adding
.trim()tostringArray[i]in the String fallback path. This is an improvement over the original code which did not trim. No change needed, just noting it.
Docs: The EN/ZH driver URL updates look correct and consistent. The zh source doc table formatting was also cleaned up which is fine.
Build is currently pending — will check CI once it completes. Looking forward to the next iteration!
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed the latest head 30ebcf4f96f7 from scratch and checked the YashanDB vector conversion path again on the new revision.
What this PR fixes
- User pain: the older YashanDB JDBC path does not handle the newer vector APIs and vector type mapping cleanly.
- Fix approach: upgrade the driver, add explicit vector-type mapping (including
DOUBLE_VECTOR), and wire the YashanDB row converter through the nativecom.yashandb.vector.VectorAPI. - In one sentence: the source direction still looks good, and the latest head extends the vector support further.
Runtime path checked
read path
-> YashanDbJdbcRowConverter.toInternal(...)
-> ResultSet.getObject(...)
-> Vector instance or string fallback
-> convert to SeaTunnel vector buffer
write path
-> setValueToStatementByDataType(...)
-> Vector.ofFloat32Values(...) / Vector.ofFloat64Values(...)
-> JDBC driver writes the vector payload
Current findings
- The latest head adds the
DOUBLE_VECTORbranch consistently throughSqlType,VectorType,SeaTunnelDataTypeConvertorUtil, and the YashanDB row converter. - I do not see a reopened source-level blocker on the latest head.
- Non-blocking note: the E2E test still updates the driver URL but does not appear to add a real vector column round-trip in
JdbcYashanDbIT, so the native vector path is still not exercised end to end. I would still recommend adding that later, but I am not treating it as the blocker on this head.
Current gate snapshot
Build: FAILURE https://github.com/apache/seatunnel/runs/86346313573- linked test-results summary currently points to the contributor fork run: https://github.com/Tlinian/seatunnel/actions/runs/29087857656/job/86346252084
From Daniel's side I do not see a new code blocker on the current head; the remaining gate is the current Build failure above.
069e152 to
178a2ed
Compare
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for the update. I re-reviewed the latest head from scratch, and I also want to correct one thing from my previous round: sorry, I should have framed the vector coverage and precision boundary more strongly earlier. After tracing the actual SeaTunnel ByteBuffer formats, I do see a source-side blocker now.
What this PR fixes
- User pain: YashanDB VECTOR columns are not handled cleanly with the newer JDBC driver API.
- Fix approach: upgrade the YashanDB JDBC driver to
1.10.7, read nativecom.yashandb.vector.Vectorobjects, and write YashanDB vectors throughVector.ofFloat32Values(...)/Vector.ofFloat64Values(...). - In one sentence: the direction is good, but the current implementation still corrupts
FLOAT16_VECTOR/BFLOAT16_VECTORwrites and the new vector path is not covered by E2E yet.
Full runtime path checked
Upstream SeaTunnel vector row
-> FakeDataRandomUtils.randomFloat16Vector() / randomBFloat16Vector()
-> builds Short[] half/bfloat values
-> VectorUtils.toByteBuffer(Short[]) // 2 bytes per element
JDBC sink writes to YashanDB
-> AbstractJdbcRowConverter.toExternal(...)
-> YashanDbJdbcRowConverter.setValueToStatementByDataType(...)
-> isFloatVectorType(FLOAT16_VECTOR/BFLOAT16_VECTOR) == true
-> toPrimitiveFloatArray(ByteBuffer)
-> byteBuffer.remaining() / Float.BYTES
-> byteBuffer.getFloat() // reads 4 bytes per element
-> Vector.ofFloat32Values(...)
-> YashanDB VECTOR column
Result
-> FLOAT_VECTOR: matches the existing 4-byte float buffer format
-> FLOAT16_VECTOR / BFLOAT16_VECTOR: a 2-byte encoded buffer is read as 4-byte floats, so dimensions and values are corrupted
Findings
Issue 1: FLOAT16_VECTOR / BFLOAT16_VECTOR writes read 2-byte vector elements as 4-byte floats
- Location:
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/yashandb/YashanDbJdbcRowConverter.java:173 - Description:
setValueToStatementByDataType()handlesFLOAT_VECTOR,FLOAT16_VECTOR, andBFLOAT16_VECTORthrough the sameisFloatVectorType()branch. ForByteBuffervalues it then callstoPrimitiveFloatArray(), which reads the buffer withFloat.BYTESandgetFloat(). That is valid forFLOAT_VECTOR, but SeaTunnel'sFLOAT16_VECTOR/BFLOAT16_VECTORbuffers are produced as 2-byteShort[]values viaVectorUtils.toByteBuffer(Short[]). - Risk: normal sink writes for these vector types can silently shorten the vector dimension and corrupt the values before writing to YashanDB. This is a data correctness issue.
- Suggested fix: either decode half/bfloat buffers into float32 explicitly before calling
Vector.ofFloat32Values(...), or do not claim support for these two input vector types yet and fail fast with a clear unsupported error. - Severity: High
- Already raised by others: No. This is related to @nzw921rx's review requesting full E2E coverage, but this is the concrete data-corruption path I found while tracing it.
Issue 2: the YashanDB E2E still does not exercise the new vector read/write path
- Location:
seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcYashanDbIT.java:63 - Description: +1 to @nzw921rx's requested change here.
JdbcYashanDbITstill creates only scalar columns, and this PR only updates the driver URL in that test. There is noVECTORcolumn inCREATE_SQL, no vector field infieldNames, and no vector value ininitTestData(). - Risk: the rewritten
ResultSet.getObject(...) -> Vector,Vector.ofFloat32Values(...), andVector.ofFloat64Values(...)branches are not proven against the real driver/database path. The bug in Issue 1 is exactly the kind of issue this E2E gap can miss. - Suggested fix: add real round-trip E2E coverage for at least
VECTOR(...,FLOAT32),VECTOR(...,FLOAT64), and null vector values. If this PR intends to support SeaTunnelFLOAT16_VECTOR/BFLOAT16_VECTORwrites, please cover those buffers too. - Severity: Medium
- Already raised by others: Yes, @nzw921rx review
4683826227; this review adds the concrete call-chain evidence.
Issue 3: the new core conversion helpers need comments for the buffer-format contract
- Location:
seatunnel-connectors-v2/connector-jdbc/src/main/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/yashandb/YashanDbJdbcRowConverter.java:198 - Description:
convertToVectorBuffer(),toPrimitiveFloatArray(), andsetArray()are now the key format boundary between SeaTunnel vector buffers and the YashanDB driver API, but they do not document the element width / precision assumptions. - Risk: future changes may keep treating float32, float16, bfloat16, and float64 as the same buffer shape, which makes precision bugs hard to spot.
- Suggested fix: add short comments explaining the supported buffer formats and any intentional precision downgrade.
- Severity: Medium
- Already raised by others: No.
Test coverage and stability
- Unit tests: the type converter assertions cover metadata mapping, including
VECTOR(3,FLOAT64)and ARRAY-to-VECTOR reconversion. - E2E: still missing the real vector round-trip path in
JdbcYashanDbIT. - Stability rating: Stable for the changed tests themselves. This PR does not add a new hard sleep or order-sensitive assertion; the existing YashanDB startup loop is condition-based.
- Local tests: not run. This review is based on the full diff, source call-chain tracing, and the current GitHub check state.
Current gate snapshot
Build: stillIN_PROGRESSat https://github.com/apache/seatunnel/runs/86781039496- Branch sync:
status=ahead,ahead_by=1,behind_by=0; no upstream-sync blocker observed.
五、报告是否可以 Merge 的结论
Conclusion: can merge after fixes
Blocking items that must be fixed:
- Issue 1: fix the
FLOAT16_VECTOR/BFLOAT16_VECTORwrite path so 2-byte vector buffers are not read as 4-byte floats. - Issue 2: add YashanDB vector E2E coverage for the real driver/database round-trip, in line with @nzw921rx's requested change.
Non-blocking suggestions:
- Issue 3: add comments around the new conversion helpers so the precision and buffer-format contract is clear for future maintainers.
Overall: the approach is promising and worth continuing. I would not merge this revision yet because the current implementation can write corrupted vector values for some SeaTunnel vector types, and the E2E test still does not exercise the new native YashanDB vector API path.
@nzw921rx Thanks for your suggestion! Currently, the vector-supported version of YashanDB has not been officially released to the public, which means we cannot upgrade the database version in the CI environment to run and verify these vector E2E tests at the moment.
Let me know what you think! Thank you again. |
|
Thanks for the detailed context. Given the current constraint, I would prefer the second option: add focused unit coverage for the vector conversion path now, and leave the E2E test for a follow-up once the vector-enabled YashanDB build is publicly available and runnable in CI. A skipped E2E would not really give us merge evidence on the current branch. When you update the PR, please make the unit tests cover the concrete vector types this branch claims to support, and mention clearly in the PR that the end-to-end validation depends on the unreleased database version. Once that update is pushed, I'm happy to re-review. |
- Use the YashanDB Vector API for vector reads and writes - Convert numeric arrays to FLOAT32 or FLOAT64 vectors based on element type - Upgrade the YashanDB JDBC driver to 1.10.7 - Update JDBC documentation and vector conversion tests
This PR fixes YashanDB vector type conversion in the JDBC connector.
Previously, YashanDB vector values were read and written through string-style array conversion, which could not correctly preserve the native YashanDB vector representation. This PR updates the YashanDB JDBC row converter to use the YashanDB Vector API for vector reads and writes.
Main changes:
Use com.yashandb.vector.Vector when reading and writing YashanDB vector fields.
Convert YashanDB vector values to SeaTunnel ByteBuffer values for internal vector representation.
Convert numeric arrays to YashanDB vectors according to element precision:
Handle null values for YashanDB ARRAY and vector types explicitly.
Upgrade the YashanDB JDBC driver from 1.9.24 to 1.10.7.
Update English and Chinese JDBC documentation with the new YashanDB driver URL.
The end-to-end validation depends on the unreleased database version. Now, add key cell coverage to the vector transformation path, and once the vector-supported YashanDB build is publicly available and runnable in CI, leave the E2E testing for later.
Purpose of this pull request
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
incompatible-changes.mdto describe the incompatibility caused by this PR.