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.

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

[529] Build xtable with scala version(s) #544

Merged

Conversation

rangareddy
Copy link
Contributor

Important Read

This pull request aims to address issues #529

What is the purpose of the pull request

To support building the xtable with multiple scala versions i.e 2.12 and 2.13.

Brief change log

  • Added scala12 and scala13 version properties in pom.xml file.
<scala12.version>2.12.15</scala12.version>
<scala13.version>2.13.8</scala13.version>
  • Added profiles to support scala12 and scala13
<!--Scala 2.12 Profile -->
<profile>
    <id>scala-2.12</id>
    <activation>
        <activeByDefault>true</activeByDefault>
    </activation>
    <properties>
        <scala.version>${scala12.version}</scala.version>
        <scala.version.prefix>2.12</scala.version.prefix>
    </properties>
    <build>
        <pluginManagement/>
    </build>
</profile>

<!--Scala 2.13 Profile -->
<!-- Once hudi supports scala 2.13 then enable following profile -->
<!--<profile>
    <id>scala-2.13</id>
    <activation>
        <activeByDefault>false</activeByDefault>
    </activation>
    <properties>
        <scala.version>${scala13.version}</scala.version>
        <scala.version.prefix>2.13</scala.version.prefix>
    </properties>
    <build>
        <pluginManagement>
            <plugins>
                <plugin>
                    <groupId>net.alchim31.maven</groupId>
                    <artifactId>scala-maven-plugin</artifactId>
                    <configuration>
                        <args>
                            <arg>-unchecked</arg>
                            <arg>-deprecation</arg>
                            <arg>-feature</arg>
                            <arg>-explaintypes</arg>
                            <arg>-target:jvm-1.8</arg>
                        </args>
                        <compilerPlugins/>
                    </configuration>
                </plugin>
            </plugins>
        </pluginManagement>
    </build>
</profile>-->

Verify this pull request

Run $ mvn clean install -DskipTests -U locally.

Note: Hudi does not yet support Scala 2.13 jars, so the scala-2.13 profile is commented out. Once Hudi supports Scala 2.13, we can use the following command to build xtable with Scala 2.13.

mvn clean install -DskipTests -Pscala-2.13 

References

@vinishjail97
Copy link
Contributor

Let's suffix the Scala version (2.12) to the jar name as suggested by @pjfanning
#529 (comment)

pom.xml Outdated
<scala.version>2.12.15</scala.version>
<scala.version.prefix>2.12</scala.version.prefix>
<scala12.version>2.12.15</scala12.version>
<scala13.version>2.13.8</scala13.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use Scala 2.12.20 and 2.13.14 - the latest versions of these patch lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the Spark version, we need to use a supported Scala version. Specifically, Spark 3.4 version supports Scala 2.12.17 for Scala 2.12 and Scala 2.13.8 for Scala 2.13.

Whenever I address all review comments, I will update the Scala 2.12 version to 2.12.17.

Reference:

  1. https://github.com/apache/spark/blob/branch-3.4/pom.xml#L3624

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use the latest patch level Scala compiler releases. This is what the Scala team recommend.

Copy link
Contributor

@vinishjail97 vinishjail97 left a comment

Choose a reason for hiding this comment

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

There are still some references for these jars across the repo. Can include the suffix in those places as well ? For example, website module just uses 0.2.0-SNAPSHOT without the scala suffix for utilities and core jars.

https://github.com/search?q=repo%3Aapache%2Fincubator-xtable%20xtable-utilities-&type=code.
https://github.com/search?q=repo%3Aapache%2Fincubator-xtable+xtable-core-&type=code

Dockerfile Outdated Show resolved Hide resolved
@vinishjail97
Copy link
Contributor

@pjfanning Can you review if things look okay ? This is the last release blocker for 0.2.0.

</build>
</profile>

<!--Scala 2.13 Profile -->
Copy link
Contributor

Choose a reason for hiding this comment

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

@pjfanning Do we leave this as commented out or should we clean it up ?

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

Dockerfile Outdated
@@ -23,7 +23,7 @@ WORKDIR /build
COPY ./ ./
RUN --mount=type=cache,target=/root/.m2 \
MAVEN_OPTS=-Dorg.slf4j.simpleLogger.defaultLogLevel=warn mvn -B package -DskipTests
RUN mv xtable-utilities/target/xtable-utilities-$(mvn help:evaluate -Dexpression=project.version -q -DforceStdout)-bundled.jar target/app.jar
RUN mv xtable-utilities_2.12-$(mvn help:evaluate -Dexpression=project.version -q -DforceStdout)-bundled.jar target/app.jar
Copy link
Contributor

Choose a reason for hiding this comment

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

@rangareddy I ran this locally, should this be ?
xtable-utilities/target/xtable-utilities_2.12

@vinishjail97
Copy link
Contributor

@rangareddy Let's squash before merging.

@rangareddy rangareddy force-pushed the 529_xtable_multi_scala_version_support branch from d87eb0f to a51c331 Compare September 26, 2024 16:47
@vinishjail97 vinishjail97 merged commit 56ebbc9 into apache:main Sep 26, 2024
3 checks passed
@vinishjail97 vinishjail97 mentioned this pull request Oct 3, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants