feat: replace std Instant with wasm-compatible wrapper #9189

waynexia · 2024-02-10T16:04:04Z

Which issue does this PR close?

Relates to #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

Signed-off-by: Ruihang Xia <[email protected]>

alamb

Thanks @waynexia

I took a look through the Cargo.lock in my datafusion checkout and it appears that adding instant would be a new dependency. I am always worried when we potentially increase the dependencies of DataFusion

I, would it be possible to add a wasm feature flag to datafusion and only pull in the needed libraries optionally?

Having a wasm feature flag might make it easier to disable other parts of the code that aren't compatible with WASM

For example, we could have something like

# in datafusion/common/src/wasm.rs

/// DataFusion wrapper around std::instant
/// uses std::time::Instant normally, but uses
/// instant::Instant library when targeting web assembly
struct Instant { 
...
}

🤔

It would also really be nice to get some sort of test / way to run wasm to trigger a bug without changes

I wonder if we could extend https://github.com/apache/arrow-datafusion/tree/main/datafusion/wasmtest 🤔

alamb · 2024-02-16T11:44:02Z

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

waynexia · 2024-02-18T03:08:20Z

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.

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2024-02-18T13:56:20Z

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: rust-lang/cargo#10801. I've tested that it won't be used in actual.

Signed-off-by: Ruihang Xia <[email protected]>

datafusion/wasmtest/src/lib.rs

Signed-off-by: Ruihang Xia <[email protected]>

alamb

Thanks @waynexia -- I think this looks good to me. I wonder if you want to put an "exclude" type lint in to prevent people from accidentally using std::time::Instant

Similar to how @DDtKey did this in https://github.com/apache/arrow-datafusion/blob/c439bc73b6a9ba9efa4c8a9b5d2fb6111e660e74/clippy.toml#L1-L4 / #9318

alamb · 2024-02-27T15:58:53Z

datafusion/common/src/wasm.rs

+
+//! WASM-related utilities
+
+#[cfg(target_family = "wasm")]


alamb · 2024-02-27T15:59:16Z

datafusion/wasmtest/src/lib.rs

    // Parse SQL (using datafusion-sql)
    let sql = "SELECT 2 + 37";
    let dialect = GenericDialect {}; // or AnsiDialect, or your own dialect ...
    let ast = Parser::parse_sql(&dialect, sql).unwrap();
    log(&format!("Parsed SQL: {ast:?}"));
 }
+
+#[cfg(test)]
+mod test {


💯 for the test

DDtKey · 2024-02-27T16:21:23Z

datafusion/common/src/wasm.rs

Just one question:
it's wrapper for Instant, but module name is wasm while "wasm" it or not depends on target family
A bit confusing to use wasm::Instant in code, like it's something specific, e.g https://github.com/apache/arrow-datafusion/blob/857a2e34299b7ec5be97b40a5019b15d0fc23bd0/datafusion/core/benches/parquet_query_sql.rs#L28

Shouldn't it be datafusion_common::instant::Instant instead? This way we fully encapsulate wrapper and can easily extend in the future if we wish

Shouldn't it be datafusion_common::instant::Instant instead? This way we fully encapsulate wrapper and can easily extend in the future if we wish

I agree this would be a better home

Makes sense 👍 please see c823e38

DDtKey · 2024-02-28T01:49:34Z

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

Signed-off-by: Ruihang Xia <[email protected]>

waynexia · 2024-02-28T03:53:28Z

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: edb67fa, it seems work by matching the import types and won't follow type alias 😉

Signed-off-by: Ruihang Xia <[email protected]>

datafusion/common/src/instant.rs

Co-authored-by: Artem Medvedev <[email protected]>

alamb

Thanks @waynexia and @DDtKey

alamb · 2024-02-28T12:30:12Z

clippy.toml

@@ -2,3 +2,7 @@ disallowed-methods = [
    { path = "tokio::task::spawn", reason = "To provide cancel-safety, use `SpawnedTask::spawn` instead (https://github.com/apache/arrow-datafusion/issues/6513)" },
    { path = "tokio::task::spawn_blocking", reason = "To provide cancel-safety, use `SpawnedTask::spawn` instead (https://github.com/apache/arrow-datafusion/issues/6513)" },
 ]
+
+disallowed-types = [
+    { path = "std::time::Instant", reason = "Use `datafusion_common::instant::Instant` instead for WASM compatibility" },


feat: replace std Instant with wasm-compatible wrapper

36dba53

Signed-off-by: Ruihang Xia <[email protected]>

github-actions bot added optimizer Optimizer rules core Core DataFusion crate labels Feb 10, 2024

update cli's lock file

4394ad8

Signed-off-by: Ruihang Xia <[email protected]>

alamb reviewed Feb 10, 2024

View reviewed changes

alamb marked this pull request as draft February 16, 2024 11:43

waynexia added 4 commits February 18, 2024 19:58

wrap instant in datafusion_common

a556ae6

Signed-off-by: Ruihang Xia <[email protected]>

write unit test

63872b2

Signed-off-by: Ruihang Xia <[email protected]>

Merge branch 'main' into warp-std-instant

56c7d0e

Signed-off-by: Ruihang Xia <[email protected]>

update lock file

7e3bf08

Signed-off-by: Ruihang Xia <[email protected]>

waynexia marked this pull request as ready for review February 18, 2024 13:49

format toml

cb16252

Signed-off-by: Ruihang Xia <[email protected]>

waynexia commented Feb 18, 2024

View reviewed changes

datafusion/wasmtest/src/lib.rs Outdated Show resolved Hide resolved

waynexia added 2 commits February 19, 2024 00:29

Update datafusion/wasmtest/src/lib.rs

d19c1ef

Merge branch 'main' into warp-std-instant

857a2e3

Signed-off-by: Ruihang Xia <[email protected]>

alamb mentioned this pull request Feb 26, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 26, 2024 #9345

Closed

9 tasks

alamb approved these changes Feb 27, 2024

View reviewed changes

DDtKey reviewed Feb 27, 2024

View reviewed changes

waynexia added 2 commits February 28, 2024 11:41

Merge branch 'main' into warp-std-instant

0bf8728

add clippy lint to prevent from using std Instant

edb67fa

Signed-off-by: Ruihang Xia <[email protected]>

waynexia added 2 commits February 28, 2024 11:57

rename mod name from wasm to instant

c823e38

Signed-off-by: Ruihang Xia <[email protected]>

fix existing std instant reference

d620e32

Signed-off-by: Ruihang Xia <[email protected]>

waynexia force-pushed the warp-std-instant branch from 041c362 to d620e32 Compare February 28, 2024 07:18

DDtKey approved these changes Feb 28, 2024

View reviewed changes

datafusion/common/src/instant.rs Outdated Show resolved Hide resolved

Update datafusion/common/src/instant.rs

ad0686a

Co-authored-by: Artem Medvedev <[email protected]>

alamb approved these changes Feb 28, 2024

View reviewed changes

alamb merged commit 90eddf6 into apache:main Feb 29, 2024
24 checks passed

waynexia deleted the warp-std-instant branch March 1, 2024 03:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace std Instant with wasm-compatible wrapper #9189

feat: replace std Instant with wasm-compatible wrapper #9189

waynexia commented Feb 10, 2024 •

edited

Loading

alamb left a comment

alamb commented Feb 16, 2024

waynexia commented Feb 18, 2024

waynexia commented Feb 18, 2024

alamb left a comment

alamb Feb 27, 2024

alamb Feb 27, 2024

DDtKey Feb 27, 2024 •

edited

Loading

alamb Feb 28, 2024

waynexia Feb 28, 2024

DDtKey commented Feb 28, 2024 •

edited

Loading

waynexia commented Feb 28, 2024

alamb left a comment

alamb Feb 28, 2024

feat: replace std Instant with wasm-compatible wrapper #9189

feat: replace std Instant with wasm-compatible wrapper #9189

Conversation

waynexia commented Feb 10, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

alamb commented Feb 16, 2024

waynexia commented Feb 18, 2024

waynexia commented Feb 18, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 27, 2024

Choose a reason for hiding this comment

alamb Feb 27, 2024

Choose a reason for hiding this comment

DDtKey Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

alamb Feb 28, 2024

Choose a reason for hiding this comment

waynexia Feb 28, 2024

Choose a reason for hiding this comment

DDtKey commented Feb 28, 2024 • edited Loading

waynexia commented Feb 28, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Feb 28, 2024

Choose a reason for hiding this comment

waynexia commented Feb 10, 2024 •

edited

Loading

DDtKey Feb 27, 2024 •

edited

Loading

DDtKey commented Feb 28, 2024 •

edited

Loading