Appender2.0 #1
Appender2.0 #1
Conversation
There was a problem hiding this comment.
Hi @maiadegraaf! Great work! I added my comments below. I'll also do another PR on the tests; I have only looked at the changes to the Appender.
From what I can tell, this will still require a follow-up PR on NULL handling. I also haven't checked out the NULL handling of go-duckdb.
Additionally, I added many naming nits closer to duckdb's naming conventions.
Also, very importantly, can you double-check your duckdb API calls to ensure we are destroying everything properly to avoid memory leaks? :)
There was a problem hiding this comment.
I just had a look at the tests - nice! Since you mentioned that they can run quite long, I tried to look for potential performance bottlenecks. Maybe we can increase (inner) list sizes after the changes?
Can you maybe also add a benchmark? Maybe compared to the row-wise insertion strategy?
Also, another follow-up PR needs to test UNION and MAP. Are we throwing Not Implemented errors for them currently? I think we need to include this in this PR.
# Conflicts: # appender.go # appender_test.go
Validity mask fix and changes towards idiomatic Go
More Appender changes
No description provided.