Giorgi / DuckDB.NET

Bindings and ADO.NET Provider for DuckDB
https://duckdb.net
MIT License
411 stars 69 forks source link

Managed appender #72

Closed arontsang closed 1 year ago

arontsang commented 1 year ago

Disagree from a "pit of success" point of view. The user would need to know to call the null append method as opposed to the value append method.

Problem is worse with string/varchar. Nothing stops a user passing a null string in...

On Mon, 19 Dec 2022, 16:55 Giorgi Dalakishvili, @.***> wrote:

@.**** commented on this pull request.

In DuckDB.NET.Data/DuckDBAppender.cs https://github.com/Giorgi/DuckDB.NET/pull/72#discussion_r1051948805:

  • AppendValue(time);
  • } +#endif
  • public void AppendValue(DuckDBTimeOnly time)
  • {
  • NativeMethods.Appender.DuckDBAppendTime(_appender, NativeMethods.DateTime.DuckDBToTime(time));
  • }
  • public void AppendValue(DuckDBTime time)
  • {
  • NativeMethods.Appender.DuckDBAppendTime(_appender, time);
  • }
  • endregion

That's also possible but I think having one method is easier to implement and better from user perspective too.

— Reply to this email directly, view it on GitHub https://github.com/Giorgi/DuckDB.NET/pull/72#discussion_r1051948805, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA46MLJLUY5OSVCY6ZOHJ4TWOAPGVANCNFSM6AAAAAATCLWRZQ . You are receiving this because you authored the thread.Message ID: @.***>

Giorgi commented 1 year ago

@arontsang What do you think about having only nullable methods for AppendValue ? Does having nullable and non-nullable overloads provide any benefit?

arontsang commented 1 year ago

That is a very good point. Certainly, it would reduce complexity and maintenance.

If however the point of the Appender being to be the most optimal solution to insert rows into DuckDB, the non-nullable overloads would provide a tiny perf boost.

Perhaps some benchmarking is in order.

On Mon, 19 Dec 2022, 22:31 Giorgi Dalakishvili, @.***> wrote:

@arontsang https://github.com/arontsang What do you think about having only nullable methods for AppendValue ? Does having nullable and non-nullable overloads provide any benefit?

— Reply to this email directly, view it on GitHub https://github.com/Giorgi/DuckDB.NET/pull/72#issuecomment-1357758821, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA46MLKMWRMFHY7HFKMIXU3WOBWUVANCNFSM6AAAAAATCLWRZQ . You are receiving this because you were mentioned.Message ID: @.***>

arontsang commented 1 year ago

I actually didn't think about that at all when writing the PR...tree, leaves and all that 😅

On Tue, 20 Dec 2022, 01:06 Aron Tsang, @.***> wrote:

That is a very good point. Certainly, it would reduce complexity and maintenance.

If however the point of the Appender being to be the most optimal solution to insert rows into DuckDB, the non-nullable overloads would provide a tiny perf boost.

Perhaps some benchmarking is in order.

On Mon, 19 Dec 2022, 22:31 Giorgi Dalakishvili, @.***> wrote:

@arontsang https://github.com/arontsang What do you think about having only nullable methods for AppendValue ? Does having nullable and non-nullable overloads provide any benefit?

— Reply to this email directly, view it on GitHub https://github.com/Giorgi/DuckDB.NET/pull/72#issuecomment-1357758821, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA46MLKMWRMFHY7HFKMIXU3WOBWUVANCNFSM6AAAAAATCLWRZQ . You are receiving this because you were mentioned.Message ID: @.***>