xorm/xorm
xorm/xorm
64
358
Fork 207
Code Issues 373 Pull Requests 18 Actions Releases 35 Activity

feat: YDB support in XORM #2329

Open
datbeohbbh wants to merge 9 commits from datbeohbbh/xorm:pr-ydb-support into main
pull from: datbeohbbh/xorm:pr-ydb-support
merge into: xorm:main
Conversation 0 Commits 9 Files Changed 58 +10694 -12
datbeohbbh commented 2023-09-02 03:53:05 +00:00
Contributor
Copy Link

Hi! I'm from YDB - an open source Distributed SQL Database. YDB application team know XORM is an ORM, which has rich features, supported multiple databases and think it would be great if XORM could also support YDB. I'm responsible for adding support for YDB in XORM.

I know it's a big PR, but most of it comes from tests. Below are some notes that I think might help with your review process.

review notes:

  1. package convert

    • add NullDuration type.

  2. package internal/statements

    • add various conversion from interface to sql Null type.
    • clarify return uint, int.

  3. package schemas

    • add IntervalType, NullInt16Type and NullTimeType

  4. package dialect

    • in Dialect interface, add IsRetryable(err error) . When working with distributed system like CockroachDB, YDB, client can get an transient error due to network, services unavailable, ... . With IsRetryable(err error) method, database can evaluate an error and decide to retry or not.
    • ydb.go, filter.go implement Dialect interface for YDB.

  5. package retry :

    • a helper package, which provides method for back-off and retry base on error evaluate function.

  6. engine.go:

    • Implement dumpTables for YDB
    • add Do and DoTx, which utilize IsRetryable(err error) of database dialect implementation and retry helper for retrying operations.

  7. package tests :

    • add ydbtest: integration tests for YDB. The reason I did it is that, YDB does not support auto-increment primary keys, so the common tests of XORM will not work for YDB. (Note: Some tests are taken from the common tests of XORM).

Please take your time to review. I am willing to make any modifications to match your standards. Thank you in advance!

Hi! I'm from [YDB](https://ydb.tech) - an open source Distributed SQL Database. YDB application team know XORM is an ORM, which has rich features, supported multiple databases and think it would be great if XORM could also support YDB. I'm responsible for adding support for YDB in XORM. I know it's a big PR, but most of it comes from tests. Below are some notes that I think might help with your review process. review notes: 1. package `convert` - [ ] add `NullDuration` type. 2. package `internal/statements` - [ ] add various conversion from interface to sql `Null` type. - [ ] clarify return `uint`, `int`. 3. package `schemas` - [ ] add `IntervalType`, `NullInt16Type` and `NullTimeType` 4. package `dialect` - [ ] <del>in `Dialect` interface, add `IsRetryable(err error)` . When working with distributed system like CockroachDB, YDB, client can get an transient error due to network, services unavailable, ... . With `IsRetryable(err error)` method, database can evaluate an `error` and decide to retry or not.</del> - [ ] `ydb.go`, `filter.go` implement `Dialect` interface for YDB. 5. package `retry` : - [ ] <del>a helper package, which provides method for back-off and retry base on error evaluate function. </del> 6. `engine.go`: - [ ] Implement `dumpTables` for YDB - [ ] <del>add `Do` and `DoTx`, which utilize `IsRetryable(err error)` of database dialect implementation and `retry` helper for retrying operations.</del> 7. package `tests` : - [ ] add `ydbtest`: integration tests for YDB. The reason I did it is that, YDB does not support auto-increment primary keys, so the common tests of XORM will not work for YDB. (Note: Some tests are taken from the common tests of XORM). Please take your time to review. I am willing to make any modifications to match your standards. Thank you in advance!
👍 1
datbeohbbh added 1 commit 2023-09-02 03:53:06 +00:00
squash all ydb-support commit history
Some checks failed
test ydb / test ydb (pull_request) Failing after 2m26s
Details
test cockroach / test cockroach (pull_request) Has been cancelled
Details
test mariadb / test mariadb (pull_request) Has been cancelled
Details
test mssql / test mssql (pull_request) Has been cancelled
Details
test mysql / test mysql (pull_request) Has been cancelled
Details
test mysql8 / test mysql8 (pull_request) Has been cancelled
Details
test postgres / test postgres (pull_request) Has been cancelled
Details
test sqlite / unit test & test sqlite (pull_request) Has been cancelled
Details
test tidb / test tidb (pull_request) Has been cancelled
Details
25fb2e8d95
datbeohbbh requested review from lunny 2023-09-02 04:05:48 +00:00
lunny added the
kind
feature
 label 2023-09-03 02:03:21 +00:00
lunny reviewed 2023-09-03 02:25:32 +00:00
dialects/filter.go Outdated
@ -146,0 +150,4 @@
Start int
}
func ydbSeqFilterConvertQuestionMark(sql, prefix string, start int) string {
lunny commented 2023-09-03 02:25:32 +00:00
Owner
Copy Link

Why not reuse postgresSeqFilter? We can rename that filter and share it.

Why not reuse `postgresSeqFilter`? We can rename that filter and share it.
datbeohbbh commented 2023-09-03 11:45:48 +00:00
Author
Contributor
Copy Link

I think, we should not share filter, in implementation of postgresSeqFilter it has cases for jsonb question, but YDB does not have it. Although, use the share filter doesn't make conversion produce wrong result string, but in future, change in a share filter may affect other databases.

I think, we should not share filter, in implementation of `postgresSeqFilter` it has cases for `jsonb` question, but YDB does not have it. Although, use the share filter doesn't make conversion produce wrong result string, but in future, change in a share filter may affect other databases.
lunny marked this conversation as resolved
datbeohbbh added 1 commit 2023-09-03 11:41:08 +00:00
(temp fix) ignore secure connection test
Some checks failed
test mariadb / test mariadb (pull_request) Successful in 4m54s
Details
test cockroach / test cockroach (pull_request) Successful in 7m50s
Details
test mssql / test mssql (pull_request) Successful in 5m46s
Details
test mysql / test mysql (pull_request) Successful in 4m52s
Details
test mysql8 / test mysql8 (pull_request) Successful in 5m15s
Details
test postgres / test postgres (pull_request) Successful in 5m36s
Details
test sqlite / unit test & test sqlite (pull_request) Failing after 4m55s
Details
test tidb / test tidb (pull_request) Successful in 5m19s
Details
test ydb / test ydb (pull_request) Failing after 22m56s
Details
a22b8b97a2
datbeohbbh force-pushed pr-ydb-support from a22b8b97a2 to 1af993e08d 2023-09-03 12:46:37 +00:00 Compare
datbeohbbh reviewed 2023-09-03 14:12:54 +00:00
.gitea/workflows/test-ydb.yml Outdated
@ -0,0 +42,4 @@
- name: test ydb (insecure connection)
run: TEST_YDB_SCHEME=grpc TEST_YDB_HOST=ydb:2136 TEST_YDB_DBNAME=local make test-ydb
services:
datbeohbbh commented 2023-09-03 14:12:54 +00:00
Author
Contributor
Copy Link

@lunny can you please help me to check CI, currently, test ydb failed because of timed out, reason I think connection is hung up because it can not see YDB server (or YDB server is not exist??).

@lunny can you please help me to check CI, currently, `test ydb` failed because of timed out, reason I think connection is hung up because it can not see YDB server (or YDB server is not exist??).
datbeohbbh force-pushed pr-ydb-support from 1af993e08d to 066cbc3447 2023-09-21 07:47:02 +00:00 Compare
datbeohbbh changed title from feat: YDB support in XORM to WIP: feat: YDB support in XORM 2023-09-21 09:38:12 +00:00
datbeohbbh added 8 commits 2023-09-24 10:10:13 +00:00
(fix) remove ad-hoc case for YDB
Some checks failed
test mariadb / test mariadb (pull_request) Successful in 4m48s
Details
test cockroach / test cockroach (pull_request) Successful in 7m26s
Details
test mssql / test mssql (pull_request) Successful in 5m44s
Details
test mysql / test mysql (pull_request) Successful in 4m41s
Details
test mysql8 / test mysql8 (pull_request) Successful in 4m59s
Details
test tidb / test tidb (pull_request) Successful in 5m10s
Details
test ydb / test ydb (pull_request) Failing after 22m17s
Details
60b9392a34
datbeohbbh force-pushed pr-ydb-support from 60b9392a34 to 1acadcdf7d 2023-09-27 09:59:12 +00:00 Compare
datbeohbbh changed title from WIP: feat: YDB support in XORM to feat: YDB support in XORM 2023-09-27 16:11:26 +00:00
lunny reviewed 2023-09-28 14:33:17 +00:00
README.md
@ -55,6 +55,9 @@ Drivers for Go's sql package which currently support database/sql includes:
- [github.com/mattn/go-oci8](https://github.com/mattn/go-oci8) (experiment)
- [github.com/sijms/go-ora](https://github.com/sijms/go-ora) (experiment)
* [YDB](https://github.com/ydb-platform/ydb)
lunny commented 2023-09-28 14:33:17 +00:00
Owner
Copy Link

I think we can just it's experiment supported and we can improve it in future.

I think we can just it's experiment supported and we can improve it in future.
👍 1
datbeohbbh force-pushed pr-ydb-support from 1acadcdf7d to bb4f917e64 2023-10-02 08:06:50 +00:00 Compare
lunny closed this pull request 2023-10-27 07:29:46 +00:00
lunny reopened this pull request 2023-10-27 07:40:17 +00:00
lunny changed target branch from master to main 2023-10-27 07:40:24 +00:00
Some checks failed
test cockroach / test cockroach (pull_request) Has been cancelled
Required
Details
test mariadb / test mariadb (pull_request) Has been cancelled
Required
Details
test mssql / test mssql (pull_request) Has been cancelled
Required
Details
test mysql / test mysql (pull_request) Has been cancelled
Required
Details
test mysql8 / test mysql8 (pull_request) Has been cancelled
Required
Details
test postgres / test postgres (pull_request) Has been cancelled
Required
Details
test sqlite / unit test & test sqlite (pull_request) Has been cancelled
Required
Details
test tidb / test tidb (pull_request) Has been cancelled
Required
Details
test ydb / test ydb (pull_request) Has been cancelled
Required
Details
This pull request has changes conflicting with the target branch.
  • Makefile
  • convert/interface.go
  • convert/time.go
  • core/tx.go
  • engine.go
  • go.mod
  • go.sum
  • session_delete.go
  • session_insert.go
  • session_update.go
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u pr-ydb-support:datbeohbbh-pr-ydb-support
git checkout datbeohbbh-pr-ydb-support
Sign in to join this conversation.
Reviewers
No reviewers
lunny
Labels
Clear labels   
backport/done

   
backport/v1

   
blocked
  
db
oracle

  
db
sqlserver

  
duplicate
  
feature
cache

  
frontport/done

   
frontport/main

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependencies

  
kind
docs

  
kind
driver

  
kind
enhancement

  
kind
feature

  
kind
performance

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
testing

  
need
feedback

  
need
test

  
proposal:accepted

   
RaspBerry Pi

   
regression

   
skip-changelog

   
upstream

   
wip

   
wontfix
No Label
backport/done
backport/v1
blocked
db
oracle
db
sqlserver
duplicate
feature
cache
frontport/done
frontport/main
invalid
kind
breaking
kind
bug
kind
build
kind
dependencies
kind
docs
kind
driver
kind
enhancement
kind
feature
kind
performance
kind
proposal
kind
question
kind
refactor
kind
testing
need
feedback
need
test
proposal:accepted
RaspBerry Pi
regression
skip-changelog
upstream
wip
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: xorm/xorm#2329
No description provided.
Delete Branch "datbeohbbh/xorm:pr-ydb-support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?