Copilot commented on code in PR #2018:
URL: https://github.com/apache/auron/pull/2018#discussion_r2850400486
##########
dev/reformat:
##########
@@ -52,12 +52,19 @@ fi
sparkver=spark-3.5
for celebornver in celeborn-0.5 celeborn-0.6
do
- run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink,flink-1.18 -Piceberg,iceberg-1.9
+ run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink-1.18 -Piceberg-1.9
done
-sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4)
+sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4 spark-4.0
spark-4.1)
for sparkver in "${sparkvers[@]}"
do
+ if [[ $sparkver == spark-4.* ]]; then
+ SCALA_PROFILE=scala-2.13
+ export JAVA_HOME=$(/usr/libexec/java_home -v 17)
+ else
+ SCALA_PROFILE=scala-2.12
+ export JAVA_HOME=$(/usr/libexec/java_home -v 1.8)
Review Comment:
The command `/usr/libexec/java_home` is macOS-specific and will fail on
Linux (ubuntu-24.04) where the CI runs. GitHub Actions' setup-java already sets
JAVA_HOME automatically when multiple Java versions are installed, so this line
may not be necessary. If explicit JAVA_HOME switching is needed, consider a
cross-platform approach like checking if the command exists first, or relying
on GitHub Actions' automatic JAVA_HOME management. See
dev/mvn-build-helper/build-native.sh:35-44 for an example of how the codebase
handles platform-specific logic with uname checks.
```suggestion
if [[ "$(uname)" == "Darwin" ]] && [[ -x /usr/libexec/java_home ]]; then
export JAVA_HOME=$(/usr/libexec/java_home -v 17)
fi
else
SCALA_PROFILE=scala-2.12
if [[ "$(uname)" == "Darwin" ]] && [[ -x /usr/libexec/java_home ]]; then
export JAVA_HOME=$(/usr/libexec/java_home -v 1.8)
fi
```
##########
dev/reformat:
##########
@@ -52,12 +52,19 @@ fi
sparkver=spark-3.5
for celebornver in celeborn-0.5 celeborn-0.6
do
- run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink,flink-1.18 -Piceberg,iceberg-1.9
+ run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink-1.18 -Piceberg-1.9
done
-sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4)
+sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4 spark-4.0
spark-4.1)
for sparkver in "${sparkvers[@]}"
do
+ if [[ $sparkver == spark-4.* ]]; then
+ SCALA_PROFILE=scala-2.13
+ export JAVA_HOME=$(/usr/libexec/java_home -v 17)
+ else
+ SCALA_PROFILE=scala-2.12
+ export JAVA_HOME=$(/usr/libexec/java_home -v 1.8)
+ fi
Review Comment:
The command `/usr/libexec/java_home` is macOS-specific and will fail on
Linux (ubuntu-24.04) where the CI runs. GitHub Actions' setup-java already sets
JAVA_HOME automatically when multiple Java versions are installed, so this line
may not be necessary. If explicit JAVA_HOME switching is needed, consider a
cross-platform approach like checking if the command exists first, or relying
on GitHub Actions' automatic JAVA_HOME management. See
dev/mvn-build-helper/build-native.sh:35-44 for an example of how the codebase
handles platform-specific logic with uname checks.
```suggestion
else
SCALA_PROFILE=scala-2.12
fi
if [[ "$(uname)" == "Darwin" ]] && command -v /usr/libexec/java_home
>/dev/null 2>&1; then
if [[ $sparkver == spark-4.* ]]; then
export JAVA_HOME=$(/usr/libexec/java_home -v 17)
else
export JAVA_HOME=$(/usr/libexec/java_home -v 1.8)
fi
fi
```
##########
dev/reformat:
##########
@@ -52,12 +52,19 @@ fi
sparkver=spark-3.5
for celebornver in celeborn-0.5 celeborn-0.6
do
- run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink,flink-1.18 -Piceberg,iceberg-1.9
+ run_maven -P"${sparkver}" -Pceleborn,"${celebornver}" -Puniffle,uniffle-0.10
-Ppaimon,paimon-1.2 -Pflink-1.18 -Piceberg-1.9
done
-sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4)
+sparkvers=(spark-3.0 spark-3.1 spark-3.2 spark-3.3 spark-3.4 spark-4.0
spark-4.1)
for sparkver in "${sparkvers[@]}"
do
+ if [[ $sparkver == spark-4.* ]]; then
+ SCALA_PROFILE=scala-2.13
+ export JAVA_HOME=$(/usr/libexec/java_home -v 17)
Review Comment:
The reformat script switches to JDK 17 for Spark 4.x, but the TPC-DS CI
workflow (.github/workflows/tpcds.yml:91-107) uses JDK 21 for Spark 4.0 and
4.1. While Spark 4.x requires JDK 17 as a minimum, using JDK 21 in CI provides
better consistency and forward compatibility. Consider aligning the reformat
script to use JDK 21 for Spark 4.x to match the testing environment, or
document why different JDK versions are used for formatting vs testing.
```suggestion
# Use JDK 21 for Spark 4.x to match the TPC-DS CI configuration
export JAVA_HOME=$(/usr/libexec/java_home -v 21)
```
##########
.github/workflows/style.yml:
##########
@@ -46,5 +46,11 @@ jobs:
java-version: 8
cache: 'maven'
check-latest: false
+ - name: Setup JDK 17
+ uses: actions/setup-java@v5
+ with:
+ distribution: 'adopt-hotspot'
+ java-version: 17
Review Comment:
The JDK 17 setup is missing the cache configuration that JDK 8 has (cache:
'maven' on line 47). When using multiple setup-java actions, only the last
one's cache is active. Consider consolidating both JDK setups into a single
action using a matrix or ensuring cache is configured for the version that will
be used most. Alternatively, if the reformat script needs to switch between
JDKs, the cache configuration should be on the primary JDK version used.
```suggestion
java-version: 17
cache: 'maven'
```
--
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]
