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.

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

[Feature][UDF][WASM] support wasm udf #24504

Closed
wants to merge 10 commits into from
Closed

Conversation

taptao
Copy link
Contributor

@taptao taptao commented Sep 17, 2023

Proposed changes

Issue Number: close #20267

Support wasm udf via wasmtime library.

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/test/util/wasm_manager_test.cpp Show resolved Hide resolved
be/test/util/wasm_test.cpp Outdated Show resolved Hide resolved
@dataroaring
Copy link
Contributor

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 37. Check the log or trigger a new build to see more.

be/src/runtime/exec_env.h Outdated Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/util/wasm_manager.h Show resolved Hide resolved
be/src/vec/functions/function_wasm.h Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
@taptao taptao force-pushed the wasm-udf branch 2 times, most recently from 805d34a to 34dfaf3 Compare December 10, 2023 08:59
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 35. Check the log or trigger a new build to see more.

be/src/runtime/user_function_cache.cpp Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/util/wasm_manager.cpp Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
function_json.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

sh-checker report

To get the full details, please check in the job output.

shellcheck errors

'shellcheck ' returned error 1 finding the following syntactical issues:

----------

In thirdparty/vars.sh line 494:
WASMTIME_C++_DOWNLOAD="https://github.com/bytecodealliance/wasmtime-cpp/archive/refs/tags/v9.0.0.tar.gz"
^-- SC2276 (error): This is interpreted as a command name containing '='. Bad assignment or comparison?


In thirdparty/vars.sh line 495:
WASMTIME_C++_NAME=v9.0.0.tar.gz
^-----------------------------^ SC2276 (error): This is interpreted as a command name containing '='. Bad assignment or comparison?


In thirdparty/vars.sh line 496:
WASMTIME_C++_SOURCE=wasmtime-cpp-9.0.0
^-- SC2276 (error): This is interpreted as a command name containing '='. Bad assignment or comparison?


In thirdparty/vars.sh line 497:
WASMTIME_C++_MD5SUM="b440d7efd615f1b018cd326876d8ead7"
^-- SC2276 (error): This is interpreted as a command name containing '='. Bad assignment or comparison?

For more information:
  https://www.shellcheck.net/wiki/SC2276 -- This is interpreted as a command ...
----------

You can address the above issues in one of three ways:
1. Manually correct the issue in the offending shell script;
2. Disable specific issues by adding the comment:
  # shellcheck disable=NNNN
above the line that contains the issue, where NNNN is the error code;
3. Add '-e NNNN' to the SHELLCHECK_OPTS setting in your .yml action file.



shfmt errors
'shfmt ' found no issues.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/test/util/wasm_test.cpp Show resolved Hide resolved
@taptao
Copy link
Contributor Author

taptao commented Dec 13, 2023

run buildall

@taptao taptao force-pushed the wasm-udf branch 2 times, most recently from 21c82e5 to 6896d89 Compare December 13, 2023 14:09
@taptao
Copy link
Contributor Author

taptao commented Dec 13, 2023

run buildall

@taptao taptao force-pushed the wasm-udf branch 2 times, most recently from 347f632 to 61daae1 Compare December 13, 2023 15:07
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/src/vec/functions/function_wasm.cpp Outdated Show resolved Hide resolved
@taptao
Copy link
Contributor Author

taptao commented Dec 14, 2023

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/src/vec/functions/function_wasm.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/src/util/wasm_manager.cpp Show resolved Hide resolved
be/src/vec/functions/function_wasm.cpp Show resolved Hide resolved
be/src/vec/functions/function_wasm.cpp Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

be/src/vec/functions/function_wasm.cpp Show resolved Hide resolved
class WasmFunctionManager {
private:
// wasmtime
wasmtime::Engine* engine;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use std::unique_ptr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

try (Store<Void> store = Store.withoutData()) {
try (Engine engine = store.engine();
Module module = Module.fromFile(engine, url.getFile())) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

could get the function name, input and return params from module?
Then compare it with the create statement to see if it is consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it before, but wasmtime-java jar doesn't support this method. I asked the wasmtime-java author this question, but there has been no reply so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@taptao
Copy link
Contributor Author

taptao commented Jan 29, 2024

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

bool WasmFunctionManager::DeleteFunction(std::string functionName) {
auto funcBody = funcs.find(functionName);
if (funcBody == funcs.end()) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]

be/src/util/wasm_manager.cpp:80:

-     if (funcBody == funcs.end()) {
-         return false;
-     }
-     funcs.erase(functionName);
-     return true;
+     return !funcBody == funcs.end();

return Status::OK();
}

Status FunctionWasm::execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'execute' exceeds recommended size/complexity thresholds [readability-function-size]

Status FunctionWasm::execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
                     ^
Additional context

be/src/vec/functions/function_wasm.cpp:69: 116 lines including whitespace and comments (threshold 80)

Status FunctionWasm::execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
                     ^

Status execute(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
size_t result, size_t input_rows_count, bool dry_run = false) override;

bool is_deterministic() const override { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'is_deterministic' can be made static [readability-convert-member-functions-to-static]

Suggested change
bool is_deterministic() const override { return false; }
static bool is_deterministic() override { return false; }


bool is_deterministic() const override { return false; }

bool is_deterministic_in_scope_of_query() const override { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'is_deterministic_in_scope_of_query' can be made static [readability-convert-member-functions-to-static]

Suggested change
bool is_deterministic_in_scope_of_query() const override { return false; }
static bool is_deterministic_in_scope_of_query() override { return false; }

@taptao
Copy link
Contributor Author

taptao commented Jan 30, 2024

run buildall

@amorynan
Copy link
Contributor

amorynan commented Feb 1, 2024

run buildall

@taptao
Copy link
Contributor Author

taptao commented Feb 1, 2024

reopen

@dataroaring dataroaring reopened this Feb 1, 2024
@taptao
Copy link
Contributor Author

taptao commented Feb 1, 2024

run buildall

@taptao
Copy link
Contributor Author

taptao commented Feb 1, 2024

run buildall

1 similar comment
@taptao
Copy link
Contributor Author

taptao commented Feb 1, 2024

run buildall

@taptao
Copy link
Contributor Author

taptao commented Feb 1, 2024

run buildall

@taptao taptao closed this Feb 4, 2024
@taptao
Copy link
Contributor Author

taptao commented Feb 4, 2024

Move to #30789 to view WASM PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Support Wasm UDF
5 participants