vertexclique commented on a change in pull request #7767:
URL: https://github.com/apache/arrow/pull/7767#discussion_r455177051
##########
File path: rust/arrow/src/lib.rs
##########
@@ -31,17 +31,19 @@ pub mod array;
pub mod bitmap;
pub mod buffer;
pub mod compute;
+#[cfg(not(target_arch="wasm32"))]
pub mod csv;
pub mod datatypes;
pub mod error;
-#[cfg(feature = "flight")]
+#[cfg(all(feature = "flight",not(target_arch="wasm32")))]
Review comment:
Inconsistent spacing between macro features and selections. Please
reorganize them consistently.
##########
File path: rust/arrow/src/compute/kernels/arithmetic.rs
##########
@@ -99,8 +99,8 @@ where
}
/// SIMD vectorized version of `math_op` above.
-#[cfg(all(any(target_arch = "x86", target_arch = "x86_64"), feature = "simd"))]
-fn simd_math_op<T, F>(
+#[cfg(all(any(target_arch = "x86", target_arch = "x86_64",
target_arch="wasm32"), feature = "simd"))]
+pub fn simd_math_op<T, F>(
Review comment:
These don't need to be public, they are internally selected based on
feature gates.
##########
File path: rust/datafusion/src/bin/repl.rs
##########
@@ -17,15 +17,29 @@
#![allow(bare_trait_objects)]
+#[cfg(not(target_arch="wasm32"))]
use arrow::util::pretty;
+#[cfg(not(target_arch="wasm32"))]
use clap::{crate_version, App, Arg};
+#[cfg(not(target_arch="wasm32"))]
use datafusion::error::Result;
+#[cfg(not(target_arch="wasm32"))]
use datafusion::execution::context::ExecutionContext;
+#[cfg(not(target_arch="wasm32"))]
use rustyline::Editor;
+#[cfg(not(target_arch="wasm32"))]
use std::env;
+#[cfg(not(target_arch="wasm32"))]
use std::path::Path;
+#[cfg(not(target_arch="wasm32"))]
use std::time::Instant;
+
+#[cfg(target_arch="wasm32")]
+pub fn main() {
+}
Review comment:
What is the point of not compiling the ported code? I don't see the
benefit of porting to wasm by doing this.
##########
File path: rust/datafusion/Cargo.toml
##########
@@ -46,18 +46,20 @@ cli = ["rustyline"]
[dependencies]
fnv = "1.0"
arrow = { path = "../arrow", version = "1.0.0-SNAPSHOT" }
-parquet = { path = "../parquet", version = "1.0.0-SNAPSHOT" }
sqlparser = "0.2.6"
-clap = "2.33"
-prettytable-rs = "0.8.0"
+paste = "0.1"
+
+[target.'cfg(not(target_arch="wasm32"))'.dependencies]
+parquet = { path = "../parquet", version = "1.0.0-SNAPSHOT" }
Review comment:
Don't not gate parquet if not wasm. The parquet should be also wasm
compilable. Moreover, actual dependencies shouldn't be gated under the negated
set, which makes it harder to maintain.
##########
File path: rust/arrow/src/lib.rs
##########
@@ -31,17 +31,19 @@ pub mod array;
pub mod bitmap;
pub mod buffer;
pub mod compute;
+#[cfg(not(target_arch="wasm32"))]
pub mod csv;
pub mod datatypes;
pub mod error;
-#[cfg(feature = "flight")]
+#[cfg(all(feature = "flight",not(target_arch="wasm32")))]
Review comment:
Also please run cargo fmt and cargo clippy
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
