-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat: feature signature codegen #3877
feat: feature signature codegen #3877
Conversation
SDK Test Report102 files ±0 102 suites ±0 2m 12s ⏱️ -5s Results for commit a0e5c37. ± Comparison against base commit 6b52ee5. This pull request removes 42 and adds 21 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
HybridSE Linux Test Report20 362 tests 20 360 ✅ 6m 46s ⏱️ Results for commit a0e5c37. ♻️ This comment has been updated with latest results. |
HybridSE Mac Test Report20 362 tests 20 360 ✅ 12m 48s ⏱️ Results for commit a0e5c37. ♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3877 +/- ##
============================================
+ Coverage 74.85% 74.89% +0.03%
Complexity 658 658
============================================
Files 745 746 +1
Lines 134302 134794 +492
Branches 1500 1440 -60
============================================
+ Hits 100534 100951 +417
- Misses 33464 33539 +75
Partials 304 304 ☔ View full report in Codecov by Sentry. |
hybridse/include/node/node_enum.h
Outdated
@@ -76,6 +76,7 @@ enum SqlNodeType { | |||
kUdfByCodeGenDef, | |||
kUdafDef, | |||
kLambdaDef, | |||
kVariadicUdfDef, |
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.
append new enum to end
hybridse/include/node/node_manager.h
Outdated
@@ -385,6 +385,10 @@ class NodeManager { | |||
FnDefNode *merge_func, FnDefNode *output_func); | |||
LambdaNode *MakeLambdaNode(const std::vector<ExprIdNode *> &args, | |||
ExprNode *body); | |||
VariadicUdfDefNode *MakeVariadicUdfDefNode(const std::string &name, |
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.
use the template function MakeNode
instead
using GetTypeF = | ||
typename std::function<void(node::NodeManager*, node::TypeNode**)>; | ||
|
||
// TypeAnnotatedFuncPtr can only be bulit from non-void return type |
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.
return type info will be inferred by get_return_type_func
for void functions, maybe that's because DataTypeTrait
do not support tuple ?
@@ -1225,6 +1265,20 @@ class ExternalFuncRegistryHelper : public UdfRegistryHelper { | |||
return *this; | |||
} | |||
|
|||
template <typename Ret, typename... Args> | |||
ExternalFuncRegistryHelper& with_return_args( |
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.
this likely break existing auto type infer facility . Consider support tuple type directly, or add clear note
.args_in<bool, int16_t, int32_t, int64_t, float, double>(); | ||
|
||
v1::InstanceFormatHelper<v1::GCFormat>::Register(this, "gcformat", R"( | ||
@brief Return instance in GCFormat format. |
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.
more info, GCFormat link ?
NativeValue* output) { | ||
CHECK_TRUE(arg_types.size() == args.size(), kCodegenError); | ||
CHECK_TRUE(fn->GetArgSize() == args.size(), kCodegenError); | ||
std::vector<NativeValue> cur_state_values(1 + fn->update_func().size()); |
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.
it's a reduce algorithm ? So ideally can replace list of NativeValue
into single accumulate NativeValue
?
)") | ||
.args_in<bool, int16_t, int32_t, int64_t, float, double>(); | ||
|
||
RegisterExternalTemplate<v1::Discrete>("discrete") |
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.
null, slot_number
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.
bucket_size <= 0 return null
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.
libsvm slot_number + bucket_size
hybridse/src/udf/udf_registry.h
Outdated
|
||
std::shared_ptr<UdfRegistry> init_gen; | ||
ArgSignatureTable update_gen; | ||
std::unordered_map<std::string, ArgSignatureTable> output_gen; |
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.
todo
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
Implement signature features.
Add GCFormat, CSV, LIBSVM functions.