Open JAicewizard opened 2 months ago
PS, dont merge yet, I would like a review but c allocations are handled sloppily, and often not de-allocated. Also more return types can (and should) be implemented still
@JAicewizard Thanks for the contribution. I think it makes sense to also add a lot of test cases for this feature.
@ajzo90 Those are some great changes, one thing I didn't want to do however was add a bunch of Chunk
and Vector
APIs to this library. I agree that they would be great, I think leaving this PR to as un-vectorised, we cal always add a vectorised version later.
In the design of the Chunk
and Vector
apis it would be wise to also think of a way we might be able to vectorise the Appender
API at the same time. (The scanner type for example looks a lot like something that could be used in the appender).
The first thing I will do it create benchmarks to see what the bottle-neck is, my suspicion is that it is the conversion to any
for every value, which should be removable.
I will however definitely look at how you did predicatepushdown, I do not really know what this actually does so I didn't touch it.
@JAicewizard Thanks for the contribution. I think it makes sense to also add a lot of test cases for this feature.
I will, and a benchmark as well. This APi is very much not optimal, but I am glad you're open to this PR :)
I implemented a generic interface for the vector
type which allows me to speed up the UDF by 3x, as it no longer needs to push the values into an interface (at least for primitive types).
However it adds some new functions in appender_vector.go
, so I would like a quick look at this code. (note that go does not allow generic parameters on methods).
If all is well I can implement some of the features from ajzos branch to make this feature complete!
todo list:
I force-pushed to cleanup the history. This PR ended up growing a bit out of hand, especially due to the wanting a safe API for setting the types of columns, without forcing the user to pass us a value. I think the current version is nice, as it the duckdb logical type is fully managed by go-duckdb, while also giving users a nice API.
One thing I am not entirely happy with is the integration with the existing vector
type, however I did not feel the need to implement a fully custom one just for table functions. The added code allows for setting values without having to turn them into an any
. This significantly speeds up table functions, as converting a value into any
does a heap allocation which is expensive.
I think this ready for a review.
Hi @JAicewizard, with the release of duckdb 0.10.3, we now also expose scalar UDFs in the C API: https://github.com/duckdb/duckdb/pull/11786. I am currently looking into adding support for this to go-duckdb. Your PR is a great help! 😊 I noticed that there are many functions that both implementations could benefit from, like your changes to type.go
. I am wondering on how to best organise efforts to avoid writing similar code twice. 🤔 Maybe I could work off of your PR, or maybe you have other suggestions?
I am also open to finalising this PR first, and then starting on the scalar UDFs. In that case, I can give a thorough review, or open a PR to your PR with suggestions? Let me know what you think!
Ignore part of my comments, and sorry for any confusion. The scalar UDFs are on our feature
branch, so not yet part of the release. However, they will be in the not-too-far future, haha. So I'll just draft the scalar UDFs, and review this PR with that in mind. 😄
haha I was confused about scalar UDFs in 10.3 already. I indeed wrote much of the code in a way that in the future scalar UDFs would also be able to benefit from the infrastructure. Thats also why I put the Type
and Row
in separate files.
I was also thinking about doing scalar UDFs, but you can take that on if you want. I think much of the code can be copied/used for inspiration, but I'm afraid it will be difficult to make much of the callback code shared. Most code is handling parameters and returned values which can't be shared.
After this PR I will start working on another one adding vectorised table UDFs, do you have any ideas on how to implement a nice and fast way to represent a duckdb vector? I was thinking maybe something with a method SetValues[T Any](data []T)
which sets the first len(data)
values to the specified ones, and clears the others. But I am open for ideas!
A review would definitely be highly appreciated, I can review your code as well once its done!
@taniabogatsch I renamed the files to udtf*.go
so that you can create new ones.
It might also be nice for scalar UDFs to be able to extract the type information from the function being passed like in python.
Thank you for the review! I will add some documentation as well soon-ish since that is very much still missing.
I rebased and forcepushed to integrate datachunk changes. Also added a datachunk API for tablefunctions.
@taniabogatsch Do you have write access? if so could you run CI for this? I think I fixed all the issues in CI, but I would like to make sure.
Also do you know what the next steps are to getting this merged?
@JAicewizard, I ran the CI. Regarding the next steps to getting this merged, ideally, we have the DataChunk
support in place. But I'll have another look at the current state, with the changes from main
, and maybe we can merge it sooner.
Thanks for the feedback! My final comment got lost, but I pushed my changes already anyways. I force-pushed for the rebase and re-organisation of commits
This implements table function UDFs.
The function takes in an interface, from which it detects the type of the parameters and return values. This interface also has a method used to generate new rows
The interface is quite simple, but not ideal. There is an issue that we need to obtain the type of the UDF from the user-input, but we also don't want the user to specify the duckdb types manually. This solution allows the user to return arbitrary types, and they will be matched to duckdb types automatically.
Another limitation is that the value interface only allows retrieving string and int64 types. This is only a limitation for the arguments, however it is a large limitation.