Skip to content

[Improve][Connector-V2] Improve YashanDB vector type conversion#11397

Open
Tlinian wants to merge 1 commit into
apache:devfrom
Tlinian:yashandb-vector
Open

[Improve][Connector-V2] Improve YashanDB vector type conversion#11397
Tlinian wants to merge 1 commit into
apache:devfrom
Tlinian:yashandb-vector

Conversation

@Tlinian

@Tlinian Tlinian commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

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:

    • FLOAT, INT, SMALLINT, TINYINT -> VECTOR(n,FLOAT32)
    • BIGINT, DOUBLE -> VECTOR(n,FLOAT64)
  • 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

@Tlinian

Tlinian commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

@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 DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 JdbcYashanDbIT E2E 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 new Vector API 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(), the FLOAT16_VECTOR and BFLOAT16_VECTOR cases fall through to the same code as FLOAT_VECTOR, calling vector.toFloatArray() and wrapping as Float[]. 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() to stringArray[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!

@Tlinian Tlinian changed the title [Improve][Connector-Jdbc] Add YashanDB vector type support [Improve][Connector-V2] Improve YashanDB vector type conversion Jul 10, 2026
@github-actions github-actions Bot added the api label Jul 10, 2026

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 native com.yashandb.vector.Vector API.
  • 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_VECTOR branch consistently through SqlType, 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

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.

@Tlinian Tlinian force-pushed the yashandb-vector branch 3 times, most recently from 069e152 to 178a2ed Compare July 13, 2026 09:28
@github-actions github-actions Bot removed the api label Jul 13, 2026

@nzw921rx nzw921rx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tlinian Thank you very much for supplementing the data types supported by YashanDB in the future. I think we need to add an E2E that can cover all the types currently supported. Thank you again for your contribution🚀

@DanielLeens DanielLeens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 native com.yashandb.vector.Vector objects, and write YashanDB vectors through Vector.ofFloat32Values(...) / Vector.ofFloat64Values(...).
  • In one sentence: the direction is good, but the current implementation still corrupts FLOAT16_VECTOR / BFLOAT16_VECTOR writes 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() handles FLOAT_VECTOR, FLOAT16_VECTOR, and BFLOAT16_VECTOR through the same isFloatVectorType() branch. For ByteBuffer values it then calls toPrimitiveFloatArray(), which reads the buffer with Float.BYTES and getFloat(). That is valid for FLOAT_VECTOR, but SeaTunnel's FLOAT16_VECTOR / BFLOAT16_VECTOR buffers are produced as 2-byte Short[] values via VectorUtils.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. JdbcYashanDbIT still creates only scalar columns, and this PR only updates the driver URL in that test. There is no VECTOR column in CREATE_SQL, no vector field in fieldNames, and no vector value in initTestData().
  • Risk: the rewritten ResultSet.getObject(...) -> Vector, Vector.ofFloat32Values(...), and Vector.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 SeaTunnel FLOAT16_VECTOR / BFLOAT16_VECTOR writes, 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(), and setArray() 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

五、报告是否可以 Merge 的结论

Conclusion: can merge after fixes

Blocking items that must be fixed:

  • Issue 1: fix the FLOAT16_VECTOR / BFLOAT16_VECTOR write 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.

@Tlinian

Tlinian commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@Tlinian Thank you very much for supplementing the data types supported by YashanDB in the future. I think we need to add an E2E that can cover all the types currently supported. Thank you again for your contribution🚀

@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.
To move forward, how would you prefer we handle this?

  • We can write the E2E test cases now but mark them to be skipped in CI for the time being.

  • We can add unit tests with mocked data first to cover the logic, and implement the E2E tests later once the vector version is officially released and available in CI.

Let me know what you think! Thank you again.

@DanielLeens

Copy link
Copy Markdown
Contributor

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants