Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Support built with java 1.8 #45

Merged
merged 2 commits into from
Feb 21, 2024
Merged

build: Support built with java 1.8 #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92c1ef2
support built with java 1.8
advancedxy Feb 18, 2024
d5a1b5d
review comments
advancedxy Feb 21, 2024
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
5 changes: 3 additions & 2 deletions .github/actions/java-test/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,10 @@ runs:
- name: Run Maven compile
shell: bash
run: |
./mvnw -B compile test-compile scalafix:scalafix -Psemanticdb
echo "JAVA_VERSION=${JAVA_VERSION}"
./mvnw -B compile test-compile scalafix:scalafix -Psemanticdb -Djava.version=${JAVA_VERSION}

- name: Run tests
shell: bash
run: |
SPARK_HOME=`pwd` ./mvnw -B clean install
SPARK_HOME=`pwd` ./mvnw -B clean install -Djava.version=${JAVA_VERSION}
11 changes: 8 additions & 3 deletions .github/actions/rust-test/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,20 @@ runs:
shell: bash
run: |
cd common
../mvnw -B clean compile -DskipTests
../mvnw -B clean compile -DskipTests -Djava.version=${JAVA_VERSION}

- name: Run Cargo test
shell: bash
run: |
cd core
# 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
JAVA_LD_LIBRARY_PATH=$JAVA_HOME/jre/lib/amd64/server:$JAVA_HOME/jre/lib/amd64:$JAVA_HOME/jre/lib/amd64/jli:$JAVA_HOME/jre/lib/server:$JAVA_HOME/jre/lib:$JAVA_HOME/jre/lib/jli
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
fi
RUST_BACKTRACE=1 \
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$JAVA_HOME/lib:$JAVA_HOME/lib/server:$JAVA_HOME/lib/jli \
DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:$JAVA_HOME/lib:$JAVA_HOME/lib/server:$JAVA_HOME/lib/jli \
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$JAVA_LD_LIBRARY_PATH \
DYLD_LIBRARY_PATH=$DYLD_LIBRARY_PATH:$JAVA_LD_LIBRARY_PATH \
cargo test

4 changes: 3 additions & 1 deletion .github/actions/setup-builder/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ runs:
- name: Install JDK ${{inputs.jdk-version}}
uses: actions/setup-java@v4
with:
distribution: 'adopt'
# distribution is chosen to be zulu as it still offers JDK 8 with Silicon support, which
# is not available in the adopt distribution
distribution: 'zulu'
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
java-version: ${{inputs.jdk-version}}

- name: Set JAVA_HOME
Expand Down
4 changes: 3 additions & 1 deletion .github/actions/setup-macos-builder/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ runs:
- name: Install JDK ${{inputs.jdk-version}}
uses: actions/setup-java@v4
with:
distribution: 'adopt'
# distribution is chosen to be zulu as it still offers JDK 8 with Silicon support, which
# is not available in the adopt distribution
distribution: 'zulu'
java-version: ${{inputs.jdk-version}}
architecture: ${{inputs.jdk-architecture}}

Expand Down
118 changes: 56 additions & 62 deletions .github/workflows/pr_build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,105 +35,99 @@ on:
workflow_dispatch:

env:
JAVA_VERSION: 17
RUST_VERSION: nightly

jobs:
linux-rust-test:
name: Rust test (amd64)
runs-on: ubuntu-latest
linux-test:
strategy:
matrix:
os: [ubuntu-latest]
java_version: [8, 11, 17]
test-target: [rust, java]
fail-fast: false
name: ${{ matrix.test-target }} test on ${{ matrix.os }} with java ${{ matrix.java_version }}
runs-on: ${{ matrix.os }}
container:
image: amd64/rust
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-builder
with:
rust-version: ${{env.RUST_VERSION}}
jdk-version: ${{env.JAVA_VERSION}}
jdk-version: ${{ matrix.java_version }}

- uses: actions/checkout@v4
- name: Rust test steps
- if: matrix.test-target == 'rust'
name: Rust test steps
uses: ./.github/actions/rust-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: ${{env.RUST_VERSION}}
jdk-version: ${{env.JAVA_VERSION}}

- uses: actions/checkout@v4
- name: Java test steps
- if: matrix.test-target == 'java'
name: Java test steps
uses: ./.github/actions/java-test


macos-rust-test:
name: Rust test (macos)
runs-on: macos-latest
macos-test:
strategy:
matrix:
os: [macos-latest]
java_version: [8, 11, 17]
test-target: [rust, java]
is_push_event:
- ${{ github.event_name == 'push' }}
exclude: # exclude java 11 for pull_request event
- java_version: 11
is_push_event: false
fail-fast: false
name: ${{ matrix.test-target }} test on ${{ matrix.os }} with java ${{ matrix.java_version }}
runs-on: ${{ matrix.os }}
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-macos-builder
with:
rust-version: ${{env.RUST_VERSION}}
jdk-version: ${{env.JAVA_VERSION}}
jdk-version: ${{ matrix.java_version }}

- uses: actions/checkout@v4
- name: Rust test steps
- if: matrix.test-target == 'rust'
name: Rust test steps
uses: ./.github/actions/rust-test

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

- uses: actions/checkout@v4
- name: Java test steps
- if: matrix.test-target == 'java'
name: Java test steps
uses: ./.github/actions/java-test

macos-aarch64-rust-test:
name: Rust test (macos-aarch64)
macos-aarch64-test:
strategy:
matrix:
java_version: [8, 11, 17]
test-target: [rust, java]
is_push_event:
- ${{ github.event_name == 'push' }}
exclude: # exclude java 11 for pull_request event
- java_version: 11
is_push_event: false
fail-fast: false
name: ${{ matrix.test-target }} test on macos-aarch64 with java ${{ matrix.java_version }}
runs-on: macos-14
env:
JAVA_VERSION: ${{ matrix.java_version == 8 && '1.8' || format('{0}', matrix.java_version) }}
Copy link
Member

Choose a reason for hiding this comment

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

This syntax matrix.java_version == 8 && '1.8' looks weird. It is evaluated to '1.8'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see this example: https://docs.github.com/en/actions/learn-github-actions/expressions#example

And also the actual log:
image

steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-macos-builder
with:
rust-version: ${{env.RUST_VERSION}}
jdk-version: ${{env.JAVA_VERSION}}
jdk-version: ${{ matrix.java_version }}
jdk-architecture: aarch64
protoc-architecture: aarch_64

- uses: actions/checkout@v4
- name: Rust test steps
- if: matrix.test-target == 'rust'
name: Rust test steps
uses: ./.github/actions/rust-test

macos-aarch64-java-test:
name: Java test (macos-aarch64)
runs-on: macos-14
steps:
- uses: actions/checkout@v4
- name: Setup Rust & Java toolchain
uses: ./.github/actions/setup-macos-builder
with:
rust-version: ${{env.RUST_VERSION}}
jdk-version: ${{env.JAVA_VERSION}}
jdk-architecture: aarch64
protoc-architecture: aarch_64

- uses: actions/checkout@v4
- name: Java test steps
- if: matrix.test-target == 'java'
name: Java test steps
uses: ./.github/actions/java-test
2 changes: 1 addition & 1 deletion common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ under the License.
<plugin>
<groupId>io.github.git-commit-id</groupId>
<artifactId>git-commit-id-maven-plugin</artifactId>
<version>5.0.0</version>
<version>4.9.9</version>
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
<executions>
<execution>
<id>get-the-git-infos</id>
Expand Down
4 changes: 4 additions & 0 deletions common/src/main/java/org/apache/comet/parquet/FileReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,6 +882,8 @@ private class ConsecutivePartList {
private final SQLMetric readThroughput;

/**
* Constructor
advancedxy marked this conversation as resolved.
Show resolved Hide resolved
*
* @param offset where the first chunk starts
*/
ConsecutivePartList(long offset) {
Expand Down Expand Up @@ -1104,6 +1106,8 @@ private void setReadMetrics(long startNs) {
}

/**
* End position of the last byte of these chunks
*
* @return the position following the last byte of these chunks
*/
public long endPos() {
Expand Down
4 changes: 2 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ under the License.
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<java.version>11</java.version>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
<maven.compiler.source>${java.version}</maven.compiler.source>
<maven.compiler.target>${java.version}</maven.compiler.target>
<scala.version>2.12.17</scala.version>
<scala.binary.version>2.12</scala.binary.version>
<scala.plugin.version>4.7.2</scala.plugin.version>
Expand Down
2 changes: 1 addition & 1 deletion spark/src/test/scala/org/apache/spark/sql/GenTPCHData.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ object GenTPCHData {
// Generate data
// Since dbgen may uses stdout to output the data, tables.genData needs to run table by table
val tableNames =
if (config.tableFilter.isBlank) tables.tables.map(_.name) else Seq(config.tableFilter)
if (config.tableFilter.trim.isEmpty) tables.tables.map(_.name) else Seq(config.tableFilter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

java8 compatible.

Copy link

Choose a reason for hiding this comment

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

How about org.apache.commons.lang3.StringUtils.isBlank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trim.isBlank is sufficient. I don't think we should use StringUtils.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I would avoid using apache commons if the java standard library has to way to achieve the same. Unless of course there is a measurable performance gain :).

tableNames.foreach { tableName =>
tables.genData(
location = s"${config.location}/tpch/sf${config.scaleFactor}_${config.format}",
Expand Down