-
Notifications
You must be signed in to change notification settings - Fork 164
Implement NewQueryAppender #545
Conversation
mlafeldt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general 👍 We could improve validation and testing of edge cases though.
|
Thanks for the review, I'll push more testing! And a design question: Should we have two new API functions (or an additional parameter) to distinguish between the user providing the column types (as we do now), or us inferring them from the first call to The pro of the first (current) approach is that you can fix in your column types, and then pass values that can be auto-cast (on the Go side) to these. The pro of the second approach is a slimmer API side, and more flexibility for the user. Basically assuming we can take any input types, and create the appender table from them, and let DuckDB's auto-casting do the job for us. And I think to infer the types, we can use existing code from parameter binding, for example. |
|
Having a second function is a good idea IMO. I'm not familiar with the inference options, but |
|
I'll move this to draft until we've merged all required changes to infer types. Related PR: #552 |
# Conflicts: # statement_test.go # type.go # types.go # value.go
|
I've hit a bit of a "blocker" with type inference here, if no type info slice is provided - if there are |
Propagates the changes in duckdb/duckdb#18738 to go-duckdb.
Here is a code snippet on how to do an
UPSERT. For more details, check out the newTestAppenderUpserttest.