Skip to content
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.

Sign up for GitHub

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

Jump to bottom

create CAST(item, type) function #2326

Merged
merged 1 commit into from
Nov 6, 2023
Merged

create CAST(item, type) function #2326

merged 1 commit into from
Nov 6, 2023

Conversation

AEsir777
Copy link
Collaborator

@AEsir777 AEsir777 commented Nov 1, 2023 •
edited
Loading

  • add bind csv config
  • add function CAST

@AEsir777 AEsir777 changed the title Bind csvconfig create CAST(item, type) function Nov 1, 2023
@AEsir777 AEsir777 force-pushed the bind-csvconfig branch 4 times, most recently from 944a80e to 791d77b Compare November 2, 2023 14:58
@AEsir777 AEsir777 force-pushed the bind-csvconfig branch 5 times, most recently from e45a4e2 to 4575089 Compare November 3, 2023 16:01
@codecov Codecov
Copy link

codecov bot commented Nov 3, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4194475) 90.24% compared to head (0da2553) 90.26%.
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2326      +/-   ##
==========================================
+ Coverage   90.24%   90.26%   +0.02%     
==========================================
  Files        1011     1011              
  Lines       32308    32344      +36     
==========================================
+ Hits        29156    29195      +39     
+ Misses       3152     3149       -3
Files Coverage Δ
src/binder/bind/bind_graph_pattern.cpp 91.61% <100.00%> (+0.05%) ⬆️
...inder/bind_expression/bind_function_expression.cpp 96.83% <100.00%> (+0.06%) ⬆️
src/binder/expression/function_expression.cpp 100.00% <100.00%> (ø)
src/c_api/value.cpp 87.83% <100.00%> (ø)
src/common/types/ku_string.cpp 100.00% <100.00%> (ø)
src/expression_evaluator/function_evaluator.cpp 100.00% <100.00%> (ø)
src/function/built_in_functions.cpp 100.00% <100.00%> (ø)
src/function/cast_string_to_functions.cpp 99.75% <100.00%> (-0.25%) ⬇️
src/function/vector_blob_functions.cpp 85.71% <100.00%> (ø)
src/function/vector_cast_functions.cpp 75.00% <ø> (ø)
... and 26 more

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AEsir777 AEsir777 marked this pull request as ready for review November 3, 2023 19:09
@AEsir777 AEsir777 force-pushed the bind-csvconfig branch 2 times, most recently from 832112e to 3d082dc Compare November 6, 2023 13:56
@AEsir777 AEsir777 force-pushed the bind-csvconfig branch 5 times, most recently from 0d05af0 to d3a050c Compare November 6, 2023 15:30
acquamarin
acquamarin approved these changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@acquamarin acquamarin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, i will let @andyfengHKU comment on the match function. Seems we have a lot of if statement for cast functions in the binder.

dataset/load-from-test/union/union_correct1.csv Outdated Show resolved Hide resolved
src/expression_evaluator/function_evaluator.cpp Show resolved Hide resolved
src/expression_evaluator/function_evaluator.cpp
if (execFunc != nullptr) {
execFunc(parameters, *resultVector);
execFunc(parameters, *resultVector, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make the dataptr default type to null, so you don't need to pass in the nullptr there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since
using scalar_exec_func = std::function<void( const std::vector<std::shared_ptr<common::ValueVector>>&, common::ValueVector&, void*)>;
specifies the function needs to take in 3 arguments, I still need to pass nullptr.

andyfengHKU
andyfengHKU approved these changes Nov 6, 2023
View reviewed changes
src/function/built_in_functions.cpp
@@ -68,6 +68,9 @@ Function* BuiltInFunctions::matchScalarFunction(
uint32_t minCost = UINT32_MAX;
for (auto& function : functionSet) {
auto func = reinterpret_cast<Function*>(function.get());
if (name == CAST_FUNC_NAME) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this. And inside bindScalarFunctionExpression we check functionName == "Cast" and directly get the function without going through the the matchScalarFunction

@AEsir777 AEsir777 merged commit 3e397af into master Nov 6, 2023
12 checks passed
@AEsir777 AEsir777 deleted the bind-csvconfig branch November 6, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyfengHKU andyfengHKU andyfengHKU approved these changes

@acquamarin acquamarin acquamarin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AEsir777 @andyfengHKU @acquamarin