refactor(expr): make evaluation async

[wangrunji0408]

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

This PR makes all evaluation functions async. It prevents blocking when evaluating UDFs and allows adding blocking pg functions in the future.

In detail, this PR changed the following funcions in trait to be async and refactored all their implementations.

#[async_trait]
pub trait Expression {
    async fn eval_checked(&self, input: &DataChunk) -> Result<ArrayRef>;
    async fn eval(&self, input: &DataChunk) -> Result<ArrayRef>;
    async fn eval_row(&self, input: &OwnedRow) -> Result<Datum>;
    ...
}

#[async_trait]
pub trait Aggregator {
    async fn update_single(&mut self, input: &DataChunk, row_id: usize) -> Result<()>;
    async fn update_multi(&mut self, input: &DataChunk, start_row_id: usize, end_row_id: usize) -> Result<()>;
    ...
}

It's worth notice that introducing async-trait results in an extra memory allocation for each call. It may lead to performance issue when the function is frequently called.

Checklist For Contributors

• I have written necessary rustdoc comments
• I have added necessary unit tests and integration tests
• I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
• I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
• All checks passed in ./risedev check (or alias, ./risedev c)

Checklist For Reviewers

• I have requested macro/micro-benchmarks as this PR can affect performance substantially, and the results are shown.

I am testing the performance and will show the result later.

Documentation

• My PR DOES NOT contain user-facing changes.

[jon-chuang]

just curious, is this zero-cost in the case where the function does not contain any actually async blocks that depend on I/O?

[xxchan]

I was also thinking about this problem. Guess it's not 🤪

[xxchan]

btw I found this article. might be interesting read
https://thenewstack.io/using-rustlangs-async-tokio-runtime-for-cpu-bound-tasks/

[wangrunji0408]

It's NOT. In fact every async function call in the async-trait creates a boxed future, which introduces dynamic memory allocation. It won't be a big problem if it is not called frequently, for example in batch evaluation. But I can't tell how much it will impact on evaluation by row. I'll do a bench for this before merging.

[jon-chuang]

Possibly due to cacheing...? https://www.reddit.com/r/rust/comments/i17arp/rust_proc_macro_attribute_execution_order/

Seems like it should be fine to write a proc_macro within a declarative macro: https://stackoverflow.com/questions/71552272/can-i-execute-a-function-inside-a-declarative-macro

What does cargo expand show?

[jon-chuang]

Alternate solution if no workaround is found is to implement async_trait desugaring manually in our macros: https://smallcultfollowing.com/babysteps/blog/2023/02/01/async-trait-send-bounds-part-1-intro/

Should not be too hard to achieve.

[wangrunji0408]

@jon-chuang Thanks for your hint! I'll have a try tomorrow.

[BugenZhao]

What about keeping the original sync expressions untouched, and introducing a new trait of AsyncExpression which is by default implemented for all Expressions with a wrapper of Box::pin? 🤔

[jon-chuang]

by default implemented for all Expressions with a wrapper of Box::pin?

I see, so the ones truly requiring async (i.e. just UDF for now) are treated as special cases with manual impl? +1 for that

Another optimization I thought of is that we can detect if an expression (or subtree) requires async, and build either the sync or async version? Anw, maybe not worth the complexity.

[wangrunji0408]

What about keeping the original sync expressions untouched, and introducing a new trait of AsyncExpression which is by default implemented for all Expressions with a wrapper of Box::pin? 🤔

It seems not that easy. As async function is contagious, all eval functions will eventually be async as long as they call child functions. Only expressions like InputRef can be kept untouched in this way. 🥹

[BugenZhao]

It seems not that easy. As async function is contagious, all eval functions will eventually be async as long as they call child functions. Only expressions like InputRef can be kept untouched in this way. 🥹

You're right. 🥵

[BugenZhao]

Rest LGTM. A great amount of work! 🥵

[wangrunji0408]

Turned it back to draft since there is still a lot of code in the executors to be refactored. 🥵

[xxchan]

😲🥵

[TennyZhuang]

Please resolve conflicts

[wangrunji0408]

Blocked by simulation test, where the workaround block_on for the gap between sync and async code no longer works...

[BugenZhao]

where the workaround block_on for the gap between sync and async code no longer works

If we ensure there's no yield point, we can use now_or_never as another workaround. 🤔

[wangrunji0408]

where the workaround block_on for the gap between sync and async code no longer works

If we ensure there's no yield point, we can use now_or_never as another workaround. 🤔

Cool idea! thanks!

[codecov]

Codecov Report

Merging #8229 (f79a273) into main (f92d7f6) will decrease coverage by 0.04%.
The diff coverage is 76.02%.

@@            Coverage Diff             @@
##             main    #8229      +/-   ##
==========================================
- Coverage   71.38%   71.34%   -0.04%     
==========================================
  Files        1147     1145       -2     
  Lines      188930   188924       -6     
==========================================
- Hits       134863   134789      -74     
- Misses      54067    54135      +68     

Flag	Coverage Δ
rust	71.34% <76.02%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/batch/src/executor/filter.rs	83.78% <ø> (ø)
src/batch/src/executor/hash_agg.rs	90.98% <ø> (ø)
src/batch/src/executor/hop_window.rs	68.18% <ø> (ø)
src/batch/src/executor/project.rs	73.07% <ø> (-0.26%)	⬇️
src/batch/src/executor/project_set.rs	75.67% <ø> (-0.17%)	⬇️
src/batch/src/executor/table_function.rs	72.72% <ø> (ø)
src/batch/src/executor/test_utils.rs	66.99% <0.00%> (ø)
src/batch/src/executor/update.rs	83.24% <ø> (-0.10%)	⬇️
src/batch/src/executor/values.rs	97.64% <ø> (ø)
src/common/src/types/mod.rs	64.89% <ø> (ø)
... and 61 more

... and 11 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[BugenZhao]

If we need to do this frequently in the future, how about making it an extension to the Expr trait?

[wangrunji0408]

It's okay. But I don't think this should be encouraged. It should only be used in constant evaluation. 🤔

[wangrunji0408]

Performance report:

Nexmark throughput

• Q10: -10%
• Q22: -10%
• Q14: +1000% (really weird 🤯)

[BugenZhao]

Q14: +1000% (really weird 🤯)

This should be caused by #8538. 🤣