datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

feat: replace std Instant with wasm-compatible wrapper

Open waynexia opened this issue 1 year ago • 3 comments

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

waynexia avatar Feb 10 '24 16:02 waynexia

Marking as draft as I don't think this PR is waiting on review anymore and I am trying to clear the review queue.

alamb avatar Feb 16 '24 11:02 alamb

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.

waynexia avatar Feb 18 '24 03:02 waynexia

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.

waynexia avatar Feb 18 '24 13:02 waynexia

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

DDtKey avatar Feb 28 '24 01:02 DDtKey

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: 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 😉

waynexia avatar Feb 28 '24 03:02 waynexia