Adding new bindings to mitigate deprecation of fields in DuckDBResult when using newer DuckDB version

[kmosegaard]

Hi

I ran into issue #25 and I set it upon myself to try to fix it and here is the result.

I've mitigated the issues by adding the new bindings recommended in the header file duckdb.h. I've also added a comment above the deprecated fields in DuckDBResult.

I'm a little uncertain if all functionality in the ADO layer is still functioning as it was before. This could probably be tested a bit more thoroughly.

I've marked the deprecated fields in DuckDBResult as private and I removed DuckDBColumn since it isn't really used anymore (except for maybe legacy usages)?

Feedback is welcome.

[kmosegaard]

I could also try to split the pull request in three different ones e.g.

• One for the new bindings
• One for passing DuckDBResult as ref everywhere
• One for updating the ADO bindings (and deprecating the other fields in the DuckDBResult struct)

I don't know. What do you think, @Giorgi?

[Giorgi]

I have three questions:

• Why did you need to add ref keyword?
• Will Marshal.PtrToStringAnsi work for columns with Unicode names?
• Why did you add a new overload for DuckDBQuery method?

[kmosegaard]

1. So, I guess, DuckDBResult is a struct which is a value type in C# thus getting copied every time you pass in to a method that doesn't explicitly say that it should be passed as ref. Correct me if I'm wrong. It's surely not a big deal.

2. Probably not, but I had to change the return value of DuckDBColumnName as I was running into memory access violations. I suspect that C# was freeing the memory. But this should be done by the native library when DuckDBResultDestroy is called as per spec. I don't know if this has changed in newer version of DuckDB.

3. Simply because I wanted to use DuckDBQuery with a string through the PlatformIndependent interface.

[Giorgi]

I think if you move the Utils class to the Bindings project you can use ToManagedString to get the column name even for Unicode names and use ToUnmanagedString in the new overload of DuckDBQuery method that takes a string query.

[Giorgi]

@kmosegaard Can you test that and update PR if that works?

[kmosegaard]

@Giorgi Why are the Windows and MacOS bindings for functions, taking DuckDBResult as a parameter, not passed by ref like the Linux bindings?

[Giorgi]

@kmosegaard To be honest I don't remember. I could have missed them when I made the change: https://github.com/Giorgi/DuckDB.NET/commit/4217d356f228e576b79d0cd22ea611e0964dd220#diff-1a375a2a87ad2e7c20a6493b86b1319706529dd36f2a47c88aece25bc2f5e252

[Giorgi]

@kmosegaard Can this be merged or does it need to be updated to be finished?

[kmosegaard]

I've updated the pull request, so that DuckDBColumnName is changed to return an IntPtr which is then converted to a ManagedString using your helper class.

I think it is ready for review.

[Giorgi]

So you added the new Apis but the DuckDB.NET.Data project doesn't use them yet, right?

[kmosegaard]

Yes, but that will come in a follow-up pull request. So, we can review those carefully.

[Giorgi]

Ok, I'll merge this PR.