Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions .github/actions/setup-builder/action.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

name: Prepare Builder
description: 'Prepare Build Environment'
inputs:
rust-version:
description: 'version of rust to install (e.g. nightly)'
required: true
default: 'nightly'
jdk-version:
description: 'jdk version to install (e.g., 17)'
required: true
default: '17'
runs:
using: "composite"
steps:
- name: Install Build Dependencies
shell: bash
run: |
apt-get update
apt-get install -y openjdk-${{inputs.jdk-version}}-jdk protobuf-compiler
- name: Setup Rust toolchain
shell: bash
# rustfmt is needed for the substrait build script
run: |
echo "Installing ${{inputs.rust-version}}"
rustup toolchain install ${{inputs.rust-version}}
rustup default ${{inputs.rust-version}}
rustup component add rustfmt clippy
128 changes: 128 additions & 0 deletions .github/workflows/pr_build.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

name: PR Build

concurrency:
group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }}
cancel-in-progress: true

on:
push:
paths-ignore:
- "doc/**"
- "**.md"
pull_request:
paths-ignore:
- "doc/**"
- "**.md"
# manual trigger
# https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow
workflow_dispatch:

env:
JAVA_VERSION: 17

jobs:
linux-rust-test:
name: Rust test (amd64)
runs-on: ubuntu-latest
container:
image: amd64/rust
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-builder
with:
rust-version: nightly
jdk-version: ${{env.JAVA_VERSION}}

- name: Check cargo fmt
run: |
cd core
cargo fmt --all -- --check --color=never

- name: Check cargo clippy
run: |
cd core
cargo clippy --color=never -- -D warnings

- name: Check compilation
run: |
cd core
cargo check --benches

- name: Setup JAVA_HOME
run: |
echo "JAVA_HOME=/usr/lib/jvm/java-${{env.JAVA_VERSION}}-openjdk-amd64" >> $GITHUB_ENV

- name: Cache Maven dependencies
uses: actions/cache@v4
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-

- name: Build common module (pre-requisite for Rust tests)
run: |
cd common
../mvnw clean compile -DskipTests

- name: Run cargo test
run: |
cd core
# This is required to run some JNI related tests on the Rust side
RUST_BACKTRACE=1 LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$JAVA_HOME/lib:$JAVA_HOME/lib/server cargo test

linux-java-test:
name: Java test (amd64)
runs-on: ubuntu-latest
container:
image: amd64/rust
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-builder
with:
rust-version: nightly
jdk-version: ${{env.JAVA_VERSION}}

- name: Run cargo build
run: |
cd core
cargo build

- name: Setup JAVA_HOME
run: |
echo "JAVA_HOME=/usr/lib/jvm/java-${{env.JAVA_VERSION}}-openjdk-amd64" >> $GITHUB_ENV

- name: Cache Maven dependencies
uses: actions/cache@v4
with:
path: ~/.m2/repository
key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
restore-keys: |
${{ runner.os }}-maven-

- name: Run Maven compile
run: |
./mvnw compile test-compile scalafix:scalafix -Psemanticdb

- name: Run tests
run: |
SPARK_HOME=`pwd` ./mvnw clean install
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we use ./mvnw verify in Makefile, do we need to do that?

Copy link
Copy Markdown
Member

@viirya viirya Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need BOSON_CONF_DIR?

Copy link
Copy Markdown
Member Author

@sunchao sunchao Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

install covers verify: https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#a-build-lifecycle-is-made-up-of-phases

COMET_CONF_DIR is optional and by default, Rust test outputs are directed to stdout.

1 change: 1 addition & 0 deletions core/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ mod tests {
/// See [`object_panic_exception`] for a test which involves generating a panic and verifying
/// that the resulting stack trace includes the offending call.
#[test]
#[ignore]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the golden file for this test is not added yet, so ignore for now

pub fn stacktrace_string() {
// Setup: Start with a backtrace that includes all of the expected scenarios, including
// cases where the file and location are not provided as part of the backtrace capture
Expand Down
22 changes: 22 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ under the License.

<!-- Reverse default (skip installation), and then enable only for child modules -->
<maven.deploy.skip>true</maven.deploy.skip>

<!-- For testing with JDK 17 -->
<extraJavaTestArgs>
-XX:+IgnoreUnrecognizedVMOptions
--add-opens=java.base/java.lang=ALL-UNNAMED
--add-opens=java.base/java.lang.invoke=ALL-UNNAMED
--add-opens=java.base/java.lang.reflect=ALL-UNNAMED
--add-opens=java.base/java.io=ALL-UNNAMED
--add-opens=java.base/java.net=ALL-UNNAMED
--add-opens=java.base/java.nio=ALL-UNNAMED
--add-opens=java.base/java.util=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent=ALL-UNNAMED
--add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED
--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED
--add-opens=java.base/sun.nio.ch=ALL-UNNAMED
--add-opens=java.base/sun.nio.cs=ALL-UNNAMED
--add-opens=java.base/sun.security.action=ALL-UNNAMED
--add-opens=java.base/sun.util.calendar=ALL-UNNAMED
-Djdk.reflect.useDirectMethodHandle=false
</extraJavaTestArgs>
</properties>

<dependencyManagement>
Expand Down Expand Up @@ -623,6 +643,7 @@ under the License.
<filereports>SparkTestSuite.txt</filereports>
<stderr/>
<tagsToExclude>org.apache.comet.IntegrationTestSuite</tagsToExclude>
<argLine>-ea -Xmx4g -Xss4m ${extraJavaTestArgs}</argLine>
<systemProperties>
<!-- emit test logs to target/unit-tests.log -->
<log4j.configurationFile>file:src/test/resources/log4j2.properties</log4j.configurationFile>
Expand Down Expand Up @@ -664,6 +685,7 @@ under the License.
<systemProperties>
<log4j.configurationFile>file:src/test/resources/log4j2.properties</log4j.configurationFile>
</systemProperties>
<argLine>-ea -Xmx4g -Xss4m ${extraJavaTestArgs}</argLine>
</configuration>
</plugin>
<plugin>
Expand Down
1 change: 1 addition & 0 deletions spark/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ under the License.
</goals>
<configuration>
<trimStackTrace>false</trimStackTrace>
<argLine>-ea ${extraJavaTestArgs}</argLine>
</configuration>
</execution>
</executions>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ class CometTPCDSV1_4_PlanStabilitySuite extends CometPlanStabilitySuite {
new File(baseResourcePath, "approved-plans-v1_4").getAbsolutePath

tpcdsQueries.foreach { q =>
test(s"check simplified (tpcds-v1.4/$q)") {
ignore(s"check simplified (tpcds-v1.4/$q)") {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends on #10

testQuery("tpcds", q)
}
}
Expand All @@ -308,7 +308,7 @@ class CometTPCDSV2_7_PlanStabilitySuite extends CometPlanStabilitySuite {
new File(baseResourcePath, "approved-plans-v2_7").getAbsolutePath

tpcdsQueriesV2_7_0.foreach { q =>
test(s"check simplified (tpcds-v2.7.0/$q)") {
ignore(s"check simplified (tpcds-v2.7.0/$q)") {
testQuery("tpcds-v2.7.0", q)
}
}
Expand Down