SEZ9 commented on PR #10988:
URL: https://github.com/apache/seatunnel/pull/10988#issuecomment-5230721333

   Thanks @DanielLeens for reviving the branch and keeping the review history 
intact — much appreciated.
   
   To keep the remaining work in one place, here's the consolidated list on 
head `8dcf4f646440`:
   
   **Merge blocker (from my earlier CHANGES_REQUESTED review, still 
unresolved):**
   - `PostgresTypeConverter.convert()` (`PostgresTypeConverter.java:249-251`) 
sets `builder.sourceType(PG_VECTOR)` as the bare literal `"vector"`. Since 
`PostgresCreateTableSqlBuilder.buildColumnSql()` prefers 
`column.getSourceType()` verbatim, auto-created sink tables lose the dimension. 
Please bake it in as `vector(N)` using the scale, matching the pattern the 
other parameterized types in this file already follow (e.g. `varchar(255)`).
   
   **Non-blocking, please fold into the same fix commit:**
   - Docs: add a pgvector mention to the Postgres JDBC connector docs (type 
mapping table + a short example).
   - Harden the dimension parsing: guard the bare `Integer.parseInt` on the 
`vector(N)` string.
   - Replace the fully-qualified `java.nio.ByteBuffer` with a proper import.
   - The regex FROM-clause fallback in `resolveTableInfo` is fragile for 
joins/subqueries and includes a MySQL-style backtick in the character class — 
at minimum drop the backtick and add a comment noting the limitation, or fail 
gracefully to the no-dimension path.
   - Fill in the PR description and checklist — they're still template 
placeholders.
   
   **CI gate:**
   - The branch is currently diverged from `dev` (ahead 12 / behind 5) and the 
latest `Build` check failed: 
https://github.com/apache/seatunnel/runs/90634507052. Since `dev` has a newer 
green run, please sync with the latest `dev` first and rerun. If it still fails 
after the sync, share the new failing check link and I'll help narrow it down.
   
   The core mapping/conversion work looks solid and the bidirectional unit 
tests are in good shape, so once the sourceType fix lands and CI is green on a 
synced head, this should be close. Thanks again for pushing it forward! 🙏
   
   <!-- streview-comment:106 -->


-- 
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]

Reply via email to