sunchao commented on code in PR #136:
URL:
https://github.com/apache/arrow-datafusion-comet/pull/136#discussion_r1507954906
##########
Makefile:
##########
@@ -21,8 +21,17 @@ all: core jvm
core:
cd core && cargo build
+test-rust:
+ # We need to compile CometException so that the cargo test can pass
+ ./mvnw compile -pl common -DskipTests $(PROFILES)
+ cd core && cargo build && \
+
LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+${LD_LIBRARY_PATH}:}${JAVA_HOME}/lib:${JAVA_HOME}/lib/server:${JAVA_HOME}/lib/jli
&& \
+
DYLD_LIBRARY_PATH=${DYLD_LIBRARY_PATH:+${DYLD_LIBRARY_PATH}:}${JAVA_HOME}/lib:${JAVA_HOME}/lib/server:${JAVA_HOME}/lib/jli
\
+ RUST_BACKTRACE=1 cargo test
jvm:
./mvnw clean package -DskipTests $(PROFILES)
+test-jvm: core
+ SPARK_HOME=`pwd` COMET_CONF_DIR=$(shell pwd)/conf RUST_BACKTRACE=1
./mvnw verify $(PROFILES)
test:
Review Comment:
Maybe we can update `test` to use `test-rust` and `test-jvm` now.
##########
DEVELOPMENT.md:
##########
@@ -47,6 +49,25 @@ A few common commands are specified in project's `Makefile`:
- `make clean`: clean up the workspace
- `bin/comet-spark-shell -d . -o spark/target/` run Comet spark shell for V1
datasources
- `bin/comet-spark-shell -d . -o spark/target/ --conf
spark.sql.sources.useV1SourceList=""` run Comet spark shell for V2 datasources
+
+## Opening Project in IDEs
Review Comment:
nit: Maybe "Development Environment"?
##########
pom.xml:
##########
@@ -531,6 +522,43 @@ under the License.
</properties>
</profile>
+ <profile>
+ <id>jdk1.8</id>
+ <activation>
+ <jdk>1.8</jdk>
+ </activation>
+ <properties>
+ <java.version>1.8</java.version>
+ <maven.compiler.source>${java.version}</maven.compiler.source>
+ <maven.compiler.target>${java.version}</maven.compiler.target>
+ <spotless.version>2.29.0</spotless.version>
Review Comment:
👍
##########
.github/actions/rust-test/action.yaml:
##########
@@ -64,9 +60,6 @@ runs:
# This is required to run some JNI related tests on the Rust side
JAVA_LD_LIBRARY_PATH=$JAVA_HOME/lib/server:$JAVA_HOME/lib:$JAVA_HOME/lib/jli
# special handing for java 1.8 for both linux and mac distributions
- if [ $JAVA_VERSION == "8" ]; then
Review Comment:
I wonder why this is no longer required
##########
.github/actions/rust-test/action.yaml:
##########
@@ -64,9 +60,6 @@ runs:
# This is required to run some JNI related tests on the Rust side
JAVA_LD_LIBRARY_PATH=$JAVA_HOME/lib/server:$JAVA_HOME/lib:$JAVA_HOME/lib/jli
# special handing for java 1.8 for both linux and mac distributions
Review Comment:
remove this line?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
