datafusion
datafusion copied to clipboard
feat: replace std Instant with wasm-compatible wrapper
Which issue does this PR close?
Relates to https://github.com/apache/arrow-datafusion/issues/7651 and #7652
Rationale for this change
std::time::Instant doesn't work on target wasm32-unknown-unknown. It would result in a runtime panic when using it. A detailed explanation can be found in the replacement wrapper crate instant's repository.
It doesn't have a difference when runs on other platforms.
What changes are included in this PR?
Replace all the occurrences of std::time::Instant with instant::Instant.
Are these changes tested?
No. I've used the compiled artifact on wasm32-unknown-unknown but I'm still trying to figure out how to write UT for that artifact.
Are there any user-facing changes?
There is no difference on platforms other than wasm32-unknown-unknown
Marking as draft as I don't think this PR is waiting on review anymore and I am trying to clear the review queue.
Thanks for your review @alamb 👍
Enclose wasm-specific dependencies and logic with a feature gate looks great to me. I would warp one Instant struct for this case.
I've extracted all the references into datafusion_common and added a target family gateway. Now the implementation of Instant will switch automatically when compiling under wasm32-unknown-unknown.
I also extended wasmtest to include unit tests, which can be run by wasm-pack test. Refer to datafusion/wasmtest/README.md for detailed commands. Noticed that I haven't added CI for it, because the --headless mode has some unknown problem and would timeout in my environment. Once I fix it we can have a CI about wasm by following the link I added in README.md.
The lock file of datafusion-cli still contains instant crate. This seems to be a bug of cargo: https://github.com/rust-lang/cargo/issues/10801. I've tested that it won't be used in actual.
I wonder if you want to put an "exclude" type lint in to prevent people from accidentally using std::time::Instant
Unfortunately, I think it's not possible "as is" for this case, because it's actually just a type alias. However new-type pattern would work (wrap Instant into new type instead of type-alias)
Somebody already tried, btw: https://github.com/rust-lang/rust-clippy/issues/10406
I wonder if you want to put an "exclude" type lint in to prevent people from accidentally using std::time::Instant
Unfortunately, I think it's not possible "as is" for this case, because it's actually just a type alias. However new-type pattern would work (wrap
Instantinto new type instead of type-alias)Somebody already tried, btw: rust-lang/rust-clippy#10406
TIL, thanks for your information.
I find disallowed_type works for our case: https://github.com/apache/arrow-datafusion/pull/9189/commits/edb67fae06bbddb080fae7e6b57b97d5236f8c56, it seems work by matching the import types and won't follow type alias 😉