search

marcboeker / go-duckdb

go-duckdb provides a database/sql driver for the DuckDB database engine.
MIT License
646 stars 97 forks source link

Appender fixes #147

Closed k-anshul closed 8 months ago

k-anshul commented 8 months ago

Discovered following issues post the changes to internal implementation of the appender API. Raising a fix for these.

  1. It was necessary to call appender.Flush otherwise results are not saved. As per ReadMe and API documentation, Flush should be an optional step.
  2. It was not possible to call appender.Flush multiple times.
  3. Previously it was possible to insert UUID via appendString and duckDB would ensure that it is casted internally. But the data chunks need to be appended with the right datatype. Using [16]byte to determine if arg is an UUID(which is not very ideal). Other option could be to introduce some other internal datatype to represent UUID. I will be happy to discuss alternatives.
marcboeker commented 8 months ago

Hey @k-anshul thanks for bringing this up. What do you think of my comments? Would this make sense?

k-anshul commented 8 months ago

Hey @marcboeker

Sorry I don't see your comments. Could you please repost ?

k-anshul commented 8 months ago

@k-anshul Sorry, forgot to submit the review 🙈

No worries :) Thanks for the review. I added a separate type now.

k-anshul commented 8 months ago

Hey @marcboeker

Thanks for merging this PR. I am discovering that few other datatypes aren't supported as well(I discovered DATE and TIME but could be others as well). The current approach in appender though efficient but has a drawback that we need to add data chunk of the exact type which is present in duckDB. This requires more knowledge of the internals of duckDB. We also need to test it extensively and export every such types from this driver and document them in appender docs. I believe we need to support both approaches i.e. what was present earlier(for ease of use since duckDB auto casts) and what is present now(for performance but not easy to use). This is similar to the APIs exposed in duckDB as well which exports both type of APIs. Please let me know what do you think. Apologies for too many back and forth on this.

marcboeker commented 8 months ago

Hi @k-anshul

Apologies for too many back and forth on this.

No worries, thanks for all your effort.

@maiadegraaf opened a PR a few days ago, which I think adds support for more data types, but it was closed shortly after.

Maybe we should coordinate the work on extending the Appender to prevent that there are two people working on the same feature.

k-anshul commented 7 months ago

Sure