Fix warnings with schema Sync2 with default varchar as NVARCHAR #1783

Merged
lunny merged 22 commits from zeripath/xorm:fix-1782-MSSQL-nvarchar-fixes into master 1 year ago
zeripath commented 1 year ago

sys.columns sets the max_length as double that which it really is for NVARCHAR and NCHAR.

Also sets Text, MediumText and LongText as NVARCHAR(MAX).

Adds a drone test running on MSSSQL tests with NVARCHAR and adjusts test cases to force them to fail with #1782

Fix #1782

sys.columns sets the max_length as double that which it really is for NVARCHAR and NCHAR. Also sets Text, MediumText and LongText as NVARCHAR(MAX). Adds a drone test running on MSSSQL tests with NVARCHAR and adjusts test cases to force them to fail with #1782 Fix #1782
zeripath added 5 commits 1 year ago
011fcf5c47
Add failing test
65547a343c
schemas.Text should map to db.defaultVarchar
558822b6d7
Fix length issue
a966473595
Set defaultVarchar to upper case
fafa19feca
add nvarchar as a testcase
zeripath added 1 commit 1 year ago
ffaa02ec28
add depends_on
zeripath added 1 commit 1 year ago
3a7eec0c08
add depends_on (2)
Poster

Hmm adding drone for mssql-nvarchar significantly lengthens your CI - I'm not certain if you want to do that.

I could just add a few choice mssql specific tests?

Hmm adding drone for mssql-nvarchar significantly lengthens your CI - I'm not certain if you want to do that. I could just add a few choice mssql specific tests?
zeripath added 1 commit 1 year ago
9b384f6f8d
Remove the duplicate mssql drone test and add a specific sync test
Poster

OK I've removed the test-mssql-nvarchar drone task and added another sync test that can be switched on or off with a flag and sets the params as appropriate.

OK I've removed the test-mssql-nvarchar drone task and added another sync test that can be switched on or off with a flag and sets the params as appropriate.
zeripath added 1 commit 1 year ago
61503d4db0
fix test
zeripath added 1 commit 1 year ago
922604eaad
prod CI
lunny reviewed 1 year ago
Dismissed
col.SQLType = schemas.SQLType{Name: schemas.TimeStampz, DefaultLength: 0, DefaultLength2: 0}
case "NVARCHAR":
col.SQLType = schemas.SQLType{Name: schemas.NVarchar, DefaultLength: 0, DefaultLength2: 0}
col.Length /= 2
lunny commented 1 year ago
Owner

Why we need that?

Why we need that?
Poster

https://social.microsoft.com/Forums/Windows/en-US/be4ad98b-473a-4183-a541-71c3fd79d958/length-of-nvarchar-column-in-system-tables?forum=transactsql

There is a column in sys.columns named max_length. This is (for almost all datatypes)
the maximum length of the column in bytes. So a column that is declared to be
varchar(10) will show 10 in that column. A column that is nvarchar(10) will show 20 in
that column since each nvarchar character takes 2 bytes. If the value of the
max_length is -1 then the column was declared as varchar(max), nvarchar(max), or
varbinary(max).

So potentially there is an issue with /2 in the case of MAXs but the test appears to work for TEXT

https://social.microsoft.com/Forums/Windows/en-US/be4ad98b-473a-4183-a541-71c3fd79d958/length-of-nvarchar-column-in-system-tables?forum=transactsql > There is a column in sys.columns named max_length. This is (for almost all datatypes) > the maximum length of the column in bytes. So a column that is declared to be > varchar(10) will show 10 in that column. A column that is nvarchar(10) will show 20 in > that column since each nvarchar character takes 2 bytes. If the value of the > max_length is -1 then the column was declared as varchar(max), nvarchar(max), or > varbinary(max). So potentially there is an issue with /2 in the case of MAXs but the test appears to work for TEXT
Poster

hehe - it worked for TEXT because I hadn't tested it... Now... it works for TEXT.

hehe - it worked for TEXT because I hadn't tested it... Now... it works for TEXT.
Poster

I've just realised I haven't tested the Text or Char places I'll get another few commits off

I've just realised I haven't tested the Text or Char places I'll get another few commits off
zeripath added 1 commit 1 year ago
c825309e5b
Add tests for Test and Char columns
zeripath added 1 commit 1 year ago
749818bddd
Properly handle MAX
zeripath added 1 commit 1 year ago
e6f2cb3daa
prod CI
zeripath added 1 commit 1 year ago
d48af2bb07
prod CI
Poster

Last CI failures are not related to this PR.

Last CI failures are not related to this PR.
zeripath added 1 commit 1 year ago
f38a19fb2b
prod CI
Owner

test-postgres and test-cockroach failures are related.

`test-postgres` and `test-cockroach` failures are related.
zeripath added 1 commit 1 year ago
Poster

Ah the changes in TestSyncTable3 have revealed a similar bug in postgres. I'm gonna push a few more test cases up in case they reveal any more interesting information.

Ah the changes in TestSyncTable3 have revealed a similar bug in postgres. I'm gonna push a few more test cases up in case they reveal any more interesting information.
zeripath added 1 commit 1 year ago
d52e8e7e8d
fix test
zeripath added 1 commit 1 year ago
2f656b2211
Postgres (and cockroachDB by inheritance) maps Char to Varchar
Poster

OK it appears that postgres and cockroachdb map Char(n)/NChar(n) as Varchar(n) because there is no benefit for char so I've just forciby mapped Char as Varchar

OK it appears that postgres and cockroachdb map Char(n)/NChar(n) as Varchar(n) because there is no benefit for char so I've just forciby mapped Char as Varchar
zeripath added 1 commit 1 year ago
lunny reviewed 1 year ago
Dismissed
res = schemas.Real
case schemas.TinyText, schemas.MediumText, schemas.LongText:
res = schemas.Text
case schemas.Char:
lunny commented 1 year ago
Owner

I don't think we should convert Char to Varchar since postgres support CHAR type. We just need

case schemas.NChar:
    res = schemas.Char
I don't think we should convert Char to Varchar since postgres support `CHAR` type. We just need ``` case schemas.NChar: res = schemas.Char ```
zeripath marked this conversation as resolved
zeripath added 2 commits 1 year ago
Poster

OK I've changed it instead. When dialect.GetColumns(...) reads a column with "character" it will get mapped to "CHAR" SQL type.

OK I've changed it instead. When `dialect.GetColumns(...)` reads a column with "character" it will get mapped to "CHAR" SQL type.
zeripath added 1 commit 1 year ago
ee850f9407
prod CI
lunny approved these changes 1 year ago
Dismissed
lunny merged commit b422930617 into master 1 year ago
lunny added the
kind/bug
label 1 year ago
lunny added this to the 1.0.5 milestone 1 year ago
zeripath deleted branch fix-1782-MSSQL-nvarchar-fixes 1 year ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as b422930617.
Sign in to join this conversation.
Loading…
There is no content yet.