-
Notifications
You must be signed in to change notification settings - Fork 2.8k
CreateVectorizedFunction using only template parameters #752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Mytherin
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.
Hey Tiago, thanks for the PR again! Some minor comments and requests :)
| //The types supported by the templated CreateVectorizedFunction | ||
| const vector<SQLType> sql_templated_types = {SQLType::BOOLEAN, SQLType::TINYINT, SQLType::SMALLINT, | ||
| SQLType::INTEGER, SQLType::BIGINT, SQLType::FLOAT, | ||
| SQLType::DOUBLE}; //, SQLType::VARCHAR |
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.
Why is the VARCHAR commented out here?
| using namespace duckdb; | ||
| using namespace std; | ||
|
|
||
| TEST_CASE("Vectorized UDF functions", "[udf_function]") { |
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.
Could you add a test with 4 or more input columns? Does not need to be for every type, one function will suffice.
| } | ||
|
|
||
| template<typename TR, typename... Args> | ||
| void CreateVectorizedFunction(string name, scalar_function_t udf_func) { |
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.
Could you add support here for vararg functions as well and add a test case for them?
Merge from CWI repo.
Merge from CWI repo.
|
I think this PR is ready, just the R package tests didn't pass due to the error: "Can't find DAV_PASSWORD in env". |
|
Indeed, this is ready to merge! Thanks for the PR again :) |
Hello all,
This PR implements the CreateVectorizedFunction using only template parameters, it's a complement to the UDF C++ API.
It's very similar to the first CreateFunction implemented by the PR #712, the main difference is in the second argument that instead of receiving a generic function pointer (i.e., TR (*udf_func)(Args…)), it receives a vectorized function pointer of the type scalar_function_t:
typedef std::function<void(DataChunk &, ExpressionState &, Vector &)> scalar_function_t;1.
template<typename TR, typename... Args> void CreateVectorizedFunction(string name, scalar_function_t udf_func)This function automatically discovers from the template typenames the corresponding SQLTypes:
An example of use would be:
P.S. It's missing a CreateVectorizedFunction to disambiguate some SQL types (some primitive types, e.g., int32_t, are mapped to the same SQLType: INTEGER, TIME and DATE). Such function will be implemented soon in the same style of the second CreateFunction present in the PR #712, with some extra SQLType arguments.