Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

uniqueidentifier formatting on sql server #439

Closed
birkmose opened this issue Feb 9, 2024 · 16 comments
Closed

uniqueidentifier formatting on sql server #439

birkmose opened this issue Feb 9, 2024 · 16 comments

Comments

@birkmose
Copy link

birkmose commented Feb 9, 2024

Hi!,

When displaying uniqueidentifier columns on SQL server the output looks binary and not formatted?

E.g.

Connected with driver sqlserver (Microsoft SQL Server 16.0.5100.7245, RTM, Azure SQL Edge Developer (64-bit))
Type "help" for help.

ms:sa@localhost/master=> \d FooTable
                    BASE TABLE "FooTable"
           Name           |       Type       | Nullable | Default 
--------------------------+------------------+----------+---------
 Id                 | uniqueidentifier | "NO"     |  
 
ms:sa@localhost/master=> SELECT TOP 1 Id from FooTable;
               Id                
---------------------------------------
 F}\xa71\x83\xe4kN\x8aU\x03/\xcc\xf8WS 
(1 row)

ms:sa@localhost/master=> SELECT TOP 1 Id, convert(nvarchar(50), Id) ConvertedId from FooTable;
               Id                |                  ConvertedId                   
---------------------------------------+--------------------------------------
 F}\xa71\x83\xe4kN\x8aU\x03/\xcc\xf8WS | 31A77D46-E483-4E6B-8A55-032FCCF85753 
(1 row)

Saw some upstream issue that might be related to this?: microsoft/go-mssqldb#92

@kenshaw
Copy link
Member

kenshaw commented Feb 9, 2024

Formatting issues like this are because that's what the underlying database driver is returning, in this case raw bytes. That gets escaped/displayed as shown. To get better formatting, try casting/converting this field. Microsoft's documentation gives this:

CONVERT(CHAR(255), id_field) AS 'char'

This should get back to usql as a string and then be displayed properly.

@kenshaw kenshaw closed this as completed Feb 9, 2024
@birkmose
Copy link
Author

birkmose commented Feb 9, 2024

Is there any way to customize (e.g. even a local build/patch/fork) usql to format it on the clientside without having to do the dance of modifying the query? I really like the look of usql, but if I get uniqueidentifiers formatted as binary blobs, that will severely limit the utility for my use-case at least. I'd have hoped that I could do stuff like select top 10 * from Foo and do exploration of datashapes.

@kenshaw
Copy link
Member

kenshaw commented Feb 9, 2024

@birkmose I've looked into the underlying type that should be returned by the driver, and it has a .String() method, so it should have been picked up by the tblfmt package properly. I'm trying to look into what the issue is, but for some reason Microsoft's SQL Server container images no longer work for me under Linux, even though I had been testing/using the same container image a week ago. I'll get to the bottom of this, as this is either an error in the underlying driver, or a minor logic issue with the tblfmt package. I can't debug/test this, however, without having a database to test against.

@kenshaw
Copy link
Member

kenshaw commented Feb 9, 2024

@birkmose I've been reading the SQL Server driver code, and it doesn't appear that it's ever allocating a UniqueIdentifier type in a call to .Scan. As such, this is likely an oversight or on purpose in the github.com/microsoft/go-mssqldb code base. I won't be able to know until I can get the container image working again.

@birkmose
Copy link
Author

birkmose commented Feb 9, 2024

This is the image i tested with on linux:

mcr.microsoft.com/azure-sql-edge:latest

@kenshaw
Copy link
Member

kenshaw commented Feb 9, 2024

Breakage: microsoft/mssql-docker#868

@kenshaw kenshaw reopened this Feb 10, 2024
@kenshaw
Copy link
Member

kenshaw commented Feb 10, 2024

@birkmose After downgrading a kernel or two, I was able to get the container to run. The type being returned from the driver is indeed []uint8. Currently, this is being displayed properly by usql. The fix to this would be for the driver to correctly return a mssql.UniqueIdentifier type. That would require a change to Microsoft's SQL Server Go driver.

@kenshaw
Copy link
Member

kenshaw commented Feb 10, 2024

@birkmose I've dug deep into the SQL Server driver code, and it is not easy to fix this. From what I can tell, there's an internal call to parseRow that is effectively dropping the type information. There's a convertAssign func (that, from my reading, would not work, regardless, as nowhere in the codebase is a UniqueIdentifier ever created). Separately, the actual convertAssign doesn't but it doesn't seem to be in path that usql, and, subsequently, tblfmt call.

I could add a hack to usql that adds formatting for []byte of specific length 16, but even then this would be extremely broken, hackish, and really not what should be done.

The SQL Server driver just isn't up to the same kind of functionality/support that other database drivers are. Other drivers will convert to the "best type" when presented with a *interface{} on a .Scan call.

That said, the ultimate resolution to this would be to politely ask Microsoft to fix this in their code. I will implement the hack and push it to the tree, but the usql codebase is being overhauled at the moment for a greater set of features, and thus I won't be able to cut a release any time soon. The hack I'll implement should be available in the next couple hours.

@kenshaw
Copy link
Member

kenshaw commented Feb 10, 2024

@birkmose I think I have a non-hacky PR that I can get the MS repo to accept, which will require no changes to usql or tblfmt, but of course will require building from source until a new usql release is cut (assuming the PR is accepted).

kenshaw added a commit to kenshaw/go-mssqldb that referenced this issue Feb 10, 2024
…ntifer for GUID types

The Rows.ColumnScanType is returning `reflect.TypeOf([]byte{})`
for GUID types (`typeGuid`). This changes it so that it instead returns
`reflect.TypeOf(UniqueIdentifier{})`, which has the knock down effects
of using packages being able to correctly ascertain the column type for
for `UniqueIdentifier`'s.

This is needed for use by `usql` (see xo/usql#439) in order to correctly
format/display GUIDs cleanly and as users expect.
@kenshaw
Copy link
Member

kenshaw commented Feb 10, 2024

@birkmose you can compile/use this from source yourself:

$ mkdir src
$ cd src
$ git clone https://github.com/kenshaw/go-mssqldb.git
$ git clone https://github.com/xo/usql.git
$ cd usql
$ go work init
$ go work use .
$ go work edit -replace github.com/microsoft/go-mssqldb=../go-mssqldb
$ go install

@birkmose
Copy link
Author

Thanks @kenshaw!

The uniqueidentifier now works, but seems that master is broken - getting errors like:
error: sqlserver: sql: Scan error on column index 2, name "SomeColumnName": converting NULL to string is unsupported

(for a column nvarchar(255) which is nullable).

@kenshaw
Copy link
Member

kenshaw commented Feb 12, 2024

@birkmose Ok; and that wasn't happening before?

@birkmose
Copy link
Author

Not from the v0.17.5 branch no. Just tested on that branch again, and works fine there :)

@kenshaw
Copy link
Member

kenshaw commented Feb 12, 2024

I'll look into it.

@kenshaw kenshaw reopened this Feb 13, 2024
@kenshaw
Copy link
Member

kenshaw commented Feb 13, 2024

@birkmose I pushed a new commit to fix this, without requiring any changes to the underlying driver. Please check it out and test it.

@birkmose
Copy link
Author

Sorry for the late answer @kenshaw , got caught up in some other stuff. I can confirm that a simple git clone https://github.com/xo/usql.git; go install works as expected - I know see uniqueidentifier formatted as expected - thank you very much!

murfffi pushed a commit to sclgo/usql that referenced this issue Mar 19, 2024
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

No branches or pull requests

2 participants