Add support for Microsoft SQL (take2) #60

jtjeferreira · 2025-01-04T06:11:28Z

Continues the work of #29

Current status: 2 tests failing

Use `SELECT TOP(?) ...` for MS SQL when there's no offset.

The built-in LogMessageWaitStrategy doesn't work.

MSSQL does not support EXCLUDE in window functions

…ing to varchar

… and filter)

…' when IDENTITY_INSERT is set to OFF" adds a prequery param to TestChecker so we can call SET IDENTITY_INSERT buyer ON before the query

…e mssql does not support sorting on aliases

jtjeferreira · 2025-01-08T05:23:20Z

I fixed/ignored a few more tests and only 2 tests are failing:

scalasql.mssql.OptionalTests.sorting.roundTripOptionalValues, when inserting a None, I get Operand type clash: varbinary is incompatible with float. I think this happens because the mssql jdbc driver fallbacks to binary. Might also be related to Fix option null values failing to put #42
scalasql.mssql.DataTypesTests.nonRoundTrip due to dates/timezones/etc

jtjeferreira · 2025-01-08T15:12:09Z

@lihaoyi I noticed scalasql.mssql.DataTypesTests scalasql.mssql.DataTypesTests.constant is only failing in scala3 and not scala2, because DataTypes.select is generating the following query:

SELECT
data_types0.my_tiny_int AS my_tiny_int, 
data_types0.my_small_int AS my_small_int, 
data_types0.my_int AS my_int, 
data_types0.my_big_int AS my_big_int, 
data_types0.my_double AS my_double, 
CASE WHEN data_types0.my_boolean THEN 1 ELSE 0 END AS my_boolean, 
data_types0.my_local_date AS my_local_date, 
data_types0.my_local_time AS my_local_time, 
data_types0.my_local_date_time AS my_local_date_time, 
data_types0.my_util_date AS my_util_date, 
data_types0.my_instant AS my_instant, 
data_types0.my_var_binary AS my_var_binary, 
data_types0.my_uuid AS my_uuid, 
data_types0.my_enum AS my_enum 
FROM data_types data_types0

which triggers the error An expression of non-boolean type specified in a context where a condition is expected, near 'THEN'.

My suspicion is that this due to different implementation of scalasql.query.TableMacros#initTableMetadata... Let me know if you have any hint...

…peMappers for java.util.Date/LocalDateTime/Instant defined in MsSqlDialect.scala

…arams was true)

lihaoyi · 2025-01-10T11:10:59Z

only 2 more failures. Seems like we're almost there!

scalasql[3.4.2].test.test 2 tests failed: 
  scalasql.mssql.DataTypesTests scalasql.mssql.DataTypesTests.constant
  scalasql.mssql.OptionalTests scalasql.mssql.OptionalTests.sorting.roundTripOptionalValues

jtjeferreira · 2025-01-10T13:19:35Z

scalasql/core/src/DbApi.scala

+          // TODO - is this necessary? Not all jdbc drivers support releaseSavepoint (MsSql)
+          // connection.releaseSavepoint(savepoint)


@lihaoyi what do you think about this?

If the underlying JDBC driver doesn't support it, there's nothing we can do, and we can just conditionally disable that line. But we should make sure we understand and document what the consequences are: does SqlServer not support savepoints at all? Or can you create them but not release them? So what does that mean for users?

I don't have any hints haha, Scala 3 support was contributed by @mrdziuban a while back. Maybe he has some ideas, but if not we'll have to investigate this and ourselves and figure out what's going on

Here are my (unsolicited) thoughts,

The common expectation is that savepoint-related resources be released after savepoint delineation. Thus, I'd argue for this behavior to be default. (Even SQLite supports releasing savepoints :)

My research also confirms that SQLServer doesn't support releaseSavepoint(). As such, we should only conditionally call releaseSavepoint() to avoid failure with SQLServer.

@aboisvert I guess you wanted to reply here, so I will reply here.

I can try to implement that conditional call to releaseSavepoint(), but I suspect at the time I faced this it was not trivial. But meanwhile I gained a bit more knowledge about scalasql. I will try again.

However what worries me is that no test failed when I commented this line. I also need to investigate that...

I don't know why no tests failed when commented out, I thought we did have pretty good tests for a variety of transaction/savepoint scenarios in https://github.com/com-lihaoyi/scalasql/blob/main/scalasql/test/src/api/TransactionTests.scala, but maybe I missed a spot

I can try to implement that conditional call to releaseSavepoint(), but I suspect at the time I faced this it was not trivial.

I revisited this today, and indeed I can't just write dialect.isInstanceOf[MsSqlDialect] because in core I don't have access to MsSqlDialect.

Shall I add an def supportSavepointRelease: Boolean to DialectConfig? Shall I add a try/catch and swallow the exception?

let's go with def supportSavepointRelease: Boolean

done in 63c538a

jtjeferreira · 2025-01-10T13:20:10Z

only 2 more failures. Seems like we're almost there!

scalasql[3.4.2].test.test 2 tests failed: 
  scalasql.mssql.DataTypesTests scalasql.mssql.DataTypesTests.constant
  scalasql.mssql.OptionalTests scalasql.mssql.OptionalTests.sorting.roundTripOptionalValues

Those are related to this comment. I think the code generated by the scala3 macro is calling this code, but not the scala2 generated code. Do you have any hint?

The scala 2.x tests are green...

lihaoyi · 2025-01-10T13:56:53Z

I don't have any hints haha, Scala 3 support was contributed by @mrdziuban a while back. Maybe he has some ideas, but if not we'll have to investigate this and ourselves and figure out what's going on

aboisvert · 2025-01-10T14:55:26Z

I don't have any hints haha, Scala 3 support was contributed by @mrdziuban a while back. Maybe he has some ideas, but if not we'll have to investigate this and ourselves and figure out what's going on

Here are my (unsolicited) thoughts,

The common expectation is that savepoint-related resources be released after savepoint delineation. Thus, I'd argue for this behavior to be default. (Even SQLite supports releasing savepoints :)
My research also confirms that SQLServer doesn't support releaseSavepoint(). As such, we should only conditionally call releaseSavepoint() to avoid failure with SQLServer.

jtjeferreira · 2025-01-10T15:16:25Z

I don't have any hints haha, Scala 3 support was contributed by @mrdziuban a while back. Maybe he has some ideas, but if not we'll have to investigate this and ourselves and figure out what's going on

No worries. I left this for the end because troubleshooting macros is always a pain...

mrdziuban · 2025-01-10T18:41:25Z

@lihaoyi I noticed scalasql.mssql.DataTypesTests scalasql.mssql.DataTypesTests.constant is only failing in scala3 and not scala2, because DataTypes.select is generating the following query:
...
CASE WHEN data_types0.my_boolean THEN 1 ELSE 0 END AS my_boolean, 
...

I've started looking into this and can reproduce it by loading a console in the test project:

./mill -i scalasql[3.4.2].test.console

then running this:

import scalasql._
import scalasql.dialects.MsSqlDialect._

case class Foo[T[_]](b: T[Boolean])
object Foo extends Table[Foo]

scalasql.example.MsSqlExample.mssqlClient.renderSql(Foo.select)
// SELECT CASE WHEN foo0.b THEN 1 ELSE 0 END AS b FROM foo foo0

What I've learned is that the CASE WHEN statement is generated here: https://github.com/jtjeferreira/scalasql/blob/mssql-joao/scalasql/src/dialects/MsSqlDialect.scala#L335

That class is constructed by implicit def ExprQueryable here: https://github.com/jtjeferreira/scalasql/blob/mssql-joao/scalasql/src/dialects/MsSqlDialect.scala#L125

And ExprQueryable is resolved by this summonInline[Queryable.Row[t2, t]] in the Scala 3 macros: https://github.com/jtjeferreira/scalasql/blob/mssql-joao/scalasql/query/src-3/TableMacro.scala#L67

I don't yet understand why ExprQueryable isn't resolved in Scala 2 by this (I thought equivalent) implicitly call: https://github.com/jtjeferreira/scalasql/blob/mssql-joao/scalasql/query/src-2/TableMacro.scala#L53

Instead Scala 2 is resolving this other implicit def ExprQueryable: https://github.com/jtjeferreira/scalasql/blob/mssql-joao/scalasql/core/src/Expr.scala#L40

So @jtjeferreira, should class ExprQueryable actually be generating the CASE WHEN statement? If so, could you change it to add the 1 = check, e.g. sql"CASE WHEN 1 = $q THEN 1 ELSE 0 END"?

…cala 2

jtjeferreira · 2025-01-15T18:46:02Z

thx @mrdziuban, your comment was very useful. I am still experimenting a few alternatives...

lihaoyi · 2025-04-04T05:30:30Z

I'm going to close this PR for now since it's been inactive for 3 months. The bounty remains open for anyone who wants to pick it up, and this PR and the previous one by @kiendang are available as starting points

@kiendang

This PR builds on the work in #29 and #60 by @kiendang and @jtjeferreira. What it does: - rebase previous works on top of current `main` - fix table name escape - fix `DataTypesTests` and `OptionalsTests` with the point of contention being the lack of a dedicated boolean column type - add more tests (inserting both values of boolean, updating BIT columns) and document any result deviations How this is achieved: T-SQL (the dialect used by MS SQL Server) does have `Boolean`s as part of filter expressions, but not as column data, with `BIT`s being used as a substitute. Hence, these uses which are both `Boolean` on the Scala side, need to be distinguished. To this end, a marker was introduced in `Context`, explicitly designating `InsertValues`, `InsertSelect`, `InsertColumns` and `Update` expression as using them as values. This was done globally, because overloading would lead to a lot more boilerplate, while simply disregarding the flags value leave the existing dialects intact. Known omissions: - `ExprBooleanOps` for `BIT` values: small edge case, it's possible and there's syntax for it (bitwise operators), but it needs another branching of the above kind, to not break the normal OR/AND/NOT syntax. Bitwise operators are also already tested as part of NumericOps. - RETURNING: this is not a thing in T-SQL, however it does have OUTPUT, which could be used to achieve similar results - ON CONFLICT: similarly, not present in T-SQL, there is MERGE in the standard, but the opinions are mixed on whether it achieves the adequate level of isolation as Postgres's ON CONFLICT does. - LATERAL JOIN: again, very Postgres, maybe CROSS APPLY could be used to achieve similar effect. - bare VALUES: not supported. However, it is not it's own `Dialect` trait yet, which would indicate this more cleanly, --------- Co-authored-by: Kien Dang <[email protected]> Co-authored-by: João Ferreira <[email protected]>

kiendang and others added 30 commits September 5, 2024 16:19

Add MSSQL deps

ec4d13d

Add MSSQL dialect

943d925

Add MSSQL Example tests

f42dabb

Add MSSQL test suite

ab34ba2

Set MSSQL_COLLATION for UTF-8

9ddd775

Use + for MS SQL string concatenation in tests

88830b9

Use datalength instead of octet_length for MS SQL in tests

13c8830

Fix some MS SQL syntaxes in tests

e9ca43f

Fix some Math functions for MS SQL

b9e52bc

Fix ORDER BY with NULL for MS SQL

98aa1b9

Add more valid results for ORDER BY with NULL tests

3a6af73

Fix LIMIT and OFFSET for MS SQL

a24e84e

Enable .take without .drop for MS SQL

9530c47

Use `SELECT TOP(?) ...` for MS SQL when there's no offset.

Fix tests involving .take and .drop for MS SQL

c2dba92

Fix MS SQL numeric ops

ec7931c

Lint

1850923

Wait for mssql container startup to complete

5aafe6e

The built-in LogMessageWaitStrategy doesn't work.

Fix wrongly edited test case

7aae613

Use a more idiomatic way of waiting for a log output message

1e7a8ab

Fix some MSSQL specific type casts

447ea7e

Add another valid expected output to test case

68c9df7

Exclude EXCLUDE test from MSSQL tests for window functions

660fb61

MSSQL does not support EXCLUDE in window functions

Fix MSSQL ORDER BY in test

bc5dde3

Merge branch 'main' into mssql

f9f99ac

Update due to internal API change

be72d63

fix isEmpty/nonEmpty

f6f9ffb

fix like, startsWith, endsWith, contains

a743d8a

fix like, startsWith, endsWith, contains but for binary blobs by cast…

ba8c418

…ing to varchar

fix WindowFunctionTests by ignoring not supported functions (nthValue…

74b1ac2

… and filter)

fix "Cannot insert explicit value for identity column in table 'buyer…

362b908

…' when IDENTITY_INSERT is set to OFF" adds a prequery param to TestChecker so we can call SET IDENTITY_INSERT buyer ON before the query

jtjeferreira added 7 commits January 8, 2025 01:13

ignore scalasql.mssql.ExprSeqOpsTests.mkString.sep test

7b9c988

ignore scalasql.mssql.SelectTests.containsMultiple test

19f42f4

simplify JoinTests.leftJoinExpr so it does not use nullsFirst, becaus…

67274df

…e mssql does not support sorting on aliases

ignore scalasql.mssql.WithCteTests.subquery test

28f5169

fix scalasql.mssql.SchemaTests

3fd8c37

scalafmt

3ce8f13

scalafix

fd74ec4

jtjeferreira added 3 commits January 8, 2025 18:19

fix scalasql.mssql.OptionalTests.sorting.roundTripOptionalValues

0728f84

use DATETIME2 in CREATE TABLE data_types as that aligns with the Ty…

56763bc

…peMappers for java.util.Date/LocalDateTime/Instant defined in MsSqlDialect.scala

fix scalasql.mssql.DataTypesTests.nonRoundTrip

ba91db2

jtjeferreira force-pushed the mssql-joao branch from 9adf9d9 to ba91db2 Compare January 8, 2025 18:19

fix a few TypeMappers overrides (it would only matter if dialectCastP…

974105c

…arams was true)

jtjeferreira commented Jan 10, 2025

View reviewed changes

jtjeferreira added 3 commits January 14, 2025 02:34

swallow releaseSavepoint exception

ff568cf

use myBoolean false instead of true to trigger test failure also in s…

9d162d8

…cala 2

supportSavepointRelease

63c538a

lihaoyi closed this Apr 4, 2025

lihaoyi mentioned this pull request Apr 20, 2025

Add support for Microsoft SQL server (1000USD Bounty) #17

Open

aldum mentioned this pull request Aug 30, 2025

MS SQL - take 3 #94

Merged

		// TODO - is this necessary? Not all jdbc drivers support releaseSavepoint (MsSql)
		// connection.releaseSavepoint(savepoint)

Add support for Microsoft SQL (take2) #60

Add support for Microsoft SQL (take2) #60

Uh oh!

Conversation

jtjeferreira commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtjeferreira commented Jan 8, 2025

Uh oh!

jtjeferreira commented Jan 8, 2025

Uh oh!

lihaoyi commented Jan 10, 2025

Uh oh!

jtjeferreira Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

lihaoyi Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

jtjeferreira Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

lihaoyi Jan 10, 2025

Choose a reason for hiding this comment

Uh oh!

jtjeferreira Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

lihaoyi Jan 14, 2025

Choose a reason for hiding this comment

Uh oh!

jtjeferreira Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

jtjeferreira commented Jan 10, 2025

Uh oh!

lihaoyi commented Jan 10, 2025

Uh oh!

aboisvert commented Jan 10, 2025

Uh oh!

jtjeferreira commented Jan 10, 2025

Uh oh!

mrdziuban commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtjeferreira commented Jan 15, 2025

Uh oh!

lihaoyi commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtjeferreira commented Jan 4, 2025 •

edited

Loading

mrdziuban commented Jan 10, 2025 •

edited

Loading