Skip to content

Thrift upgrade #54

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

Merged
merged 12 commits into from
Oct 30, 2024
Merged

Thrift upgrade #54

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
867f951
0.21
nikhil-zlai Oct 29, 2024
a028a7a
0.21
nikhil-zlai Oct 29, 2024
3cdae46
disable spark ui for catalyst util
nikhil-zlai Oct 29, 2024
67edbf5
disable spark ui for local tests
nikhil-zlai Oct 29, 2024
bfbbd14
exclude jakarta stuff
nikhil-zlai Oct 29, 2024
5f0dd1d
spark javax deps
nikhil-zlai Oct 29, 2024
86a40b6
thrift pull in
nikhil-zlai Oct 30, 2024
b7ba857
change thrift
nikhil-zlai Oct 30, 2024
bf7d112
hammer shading
nikhil-zlai Oct 30, 2024
4d3072b
kick gh actions
nikhil-zlai Oct 30, 2024
6570033
test fixes
nikhil-zlai Oct 30, 2024
cd6cf9f
comments
nikhil-zlai Oct 30, 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
2 changes: 1 addition & 1 deletion .github/image/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ RUN curl -L "https://github.com/sbt/sbt/releases/download/v1.8.2/sbt-1.8.2.tgz"
ENV PATH="/usr/local/sbt/bin:${PATH}"

# thrift
ARG THRIFT_VERSION=0.13.0
ARG THRIFT_VERSION=0.21.0
RUN apk add --no-cache \
build-base \
cmake \
Expand Down
3 changes: 2 additions & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ lazy val api = project
crossScalaVersions := supportedVersions,
libraryDependencies ++= spark_sql_provided,
libraryDependencies ++= Seq(
"org.apache.thrift" % "libthrift" % "0.13.0", // cannot upgrade this without breaking compatibility
"org.apache.thrift" % "libthrift" % "0.21.0", // cannot upgrade this without breaking compatibility
// "javax.annotation" % "javax.annotation-api" % "1.3.2",
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add shading configuration for Thrift dependency.

The PR description mentions shading, but there's no shading configuration in build.sbt. This is important to avoid dependency conflicts.

Add the following shading configuration to the api project:

assembly / assemblyShadeRules := Seq(
  ShadeRule.rename("org.apache.thrift.**" -> "shaded.org.apache.thrift.@1").inAll
)

"org.scala-lang" % "scala-reflect" % scalaVersion.value,
"org.scala-lang.modules" %% "scala-collection-compat" % "2.11.0",
"com.novocode" % "junit-interface" % "0.11" % "test",
Expand Down
2 changes: 1 addition & 1 deletion project/ThriftGen.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ object Thrift {
print_and_execute(s"rm -rf $outputPath/$cleanupSuffixPath")
s"mkdir -p $outputPath" !;
print_and_execute(s"thrift -version")
print_and_execute(s"thrift --gen $language -out $outputPath $inputPath")
print_and_execute(s"thrift --gen $language:generated_annotations=suppress -out $outputPath $inputPath")
val files = (PathFinder(new File(outputPath)) ** s"*.${Option(extension).getOrElse(language)}").get()
println("Generated files list")
files.map(_.getPath).foreach { path => println(s" $path") }
Expand Down
Loading