Skip to content

Conversation

@DavideD
Copy link
Member

@DavideD DavideD commented Jun 17, 2021

Fixes #255
Follows #852

Support for MS SQL Server.

I had to refactor the code so that we could run the creation of temporary tables as queries and not preparedQueries:
See StatementsWithParameters#execute and CreateTempTablesHandler.

I've also created a separate class for keeping the logic of the insert-and-select queries: 06528d5

We need to wait the next Vert.x SQL client release before we can merge this.

@DavideD DavideD requested a review from gavinking June 17, 2021 08:38
@DavideD DavideD added the waiting We are waiting for another PR or issue to be solved before merging this one label Jun 17, 2021
@DavideD DavideD marked this pull request as draft June 17, 2021 08:45
@DavideD DavideD force-pushed the mssql-rebase branch 2 times, most recently from 273ab24 to 7a3bdbd Compare June 17, 2021 12:37
@DavideD
Copy link
Member Author

DavideD commented Jun 17, 2021

It seems I broke something on Db2... checking

@DavideD
Copy link
Member Author

DavideD commented Jun 17, 2021

Uff... everything works fine for me locally... I will check again tomorrow.

@DavideD
Copy link
Member Author

DavideD commented Jun 21, 2021

I've found the problem... working on it

@DavideD DavideD force-pushed the mssql-rebase branch 11 times, most recently from 54d2a3f to 6bba877 Compare June 22, 2021 09:38
@DavideD
Copy link
Member Author

DavideD commented Jun 22, 2021

I think this is ready for a review now.
The failure on Db2 was happening because I wasn't ignoring the errors like in the original code.
In the end I've decided to refactor the way we handle the queries for the temporary tables because I was having a hard time understanding it. I think now it's clearer what happens and when.

I've kept the refactoring in a separate commit. @gavinking I would appreaciate if you could have a look at it.

It still need a release of the Vert.x client. @tsegismont Is there any idea about when it's going to happen?

@tsegismont
Copy link
Contributor

@DavideD we are working on bugfix release (4.1.1). At best, we would release by the end of this week.

@tsegismont
Copy link
Contributor

@DavideD @gavinking if I read correctly the build results, we have now 90% of the HR test suite passing? Do you know what's problematic in the remaining 10%? cc @vietj

@DavideD
Copy link
Member Author

DavideD commented Jun 22, 2021

@DavideD we are working on bugfix release (4.1.1). At best, we would release by the end of this week.

Sounds great

@DavideD @gavinking if I read correctly the build results, we have now 90% of the HR test suite passing? Do you know what's problematic in the remaining 10%?

It's probably less than that. We have some tests that are not db dependent and we only run them for PostgreSQL.
I will let you know which tests we skipped for MSSQL specifically

@DavideD
Copy link
Member Author

DavideD commented Jun 22, 2021

@tsegismont,
These are the only tests left: https://github.com/hibernate/hibernate-reactive/blob/32d4658035b99a389f24d1c8a1c5533d7726be9f/hibernate-reactive-core/src/test/java/org/hibernate/reactive/types/BasicTypesForSelectedDBTest.java

When testing @Lob types it just gets stuck and JsonObject causes unsupported exception.
I suppose is expected.

@tsegismont
Copy link
Contributor

SQL Server doesn't have a json data type so we must keep the corresponding tests ignored.

For @Lob, do you use an image column or binary/varbinary ?

@gavinking
Copy link
Member

@gavinking I would appreaciate if you could have a look at it.

OK let me find some time to try it out properly.

@DavideD
Copy link
Member Author

DavideD commented Jun 22, 2021

We create the following columns:

        book varchar(MAX) // @Lob String
        pic varbinary(MAX) // @Lob byte[]

and then we try to do an insert of a string or an array of bytes (length between 87000 and 100000) we pass the values as parameters of type String or BufferImpl respectively.

I said that it gets stuck but it might be just slow. The same tests doesn't take that long with the other databases though

@tsegismont
Copy link
Contributor

@DavideD I updated eclipse-vertx/vertx-sql-client#608 (comment) about varchar(max) and varbinary(max)

@tsegismont
Copy link
Contributor

tsegismont commented Jul 2, 2021

@DavideD after building eclipse-vertx/vertx-sql-client#1001 the HR test suite passes using these Gradle props:

db = MSSQL
enableMavenLocalRepo = true
vertxVersion = 4.2.0-SNAPSHOT

We need to discuss with @vietj when this fix can be delivered. In the meantime, you can workaround the issue by not running prepared queries if table does not exist.

@DavideD
Copy link
Member Author

DavideD commented Jul 4, 2021

We need to discuss with @vietj when this fix can be delivered. In the meantime, you can workaround the issue by not running prepared queries if table does not exist.

I'm not sure we can do this. Wouldn't it be possible to release a 4.1.2 with this fix? This seems like a serious issue.

@DavideD DavideD marked this pull request as ready for review July 20, 2021 15:35
@DavideD DavideD removed the waiting We are waiting for another PR or issue to be solved before merging this one label Jul 20, 2021
@DavideD
Copy link
Member Author

DavideD commented Jul 20, 2021

It seems that everything is working fine with the latest Vert.x 4.1.2.
I will merge it if there are no objections.

@gavinking
Copy link
Member

alright!

@DavideD DavideD merged commit 52a98ad into hibernate:main Jul 20, 2021
@DavideD
Copy link
Member Author

DavideD commented Jul 20, 2021

Merged!

@tsegismont
Copy link
Contributor

Awesome news!

@gavinking
Copy link
Member

Awesome news!

Indeed.

@DavideD
Copy link
Member Author

DavideD commented Jul 21, 2021

Thank you!

By the way, @tsegismont, is there a roadmap somewhere about which databases are going to be next? It would be nice to have something on the website to let users know.

I've seen a PR for Oracle, so I guess that will be next. But what's coming after that?

@tsegismont
Copy link
Contributor

There is a PR for Oracle and another for ClickHouse DB (contribution by a community member). Other than that we don't plan to support new databases soon.

@DavideD DavideD deleted the mssql-rebase branch October 12, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MS SQL Server support

3 participants