Build: Bump roaringbitmap from 1.3.0 to 1.6.0#14991
Conversation
a0f2b0f to
9b38f3d
Compare
9b38f3d to
37dee6b
Compare
37dee6b to
cd3e65b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the RoaringBitmap dependency from version 1.3.0 to 1.6.0, migrating from Maven Central (org.roaringbitmap:RoaringBitmap) to JitPack (com.github.RoaringBitmap.RoaringBitmap:roaringbitmap) as the publishing location changed after version 1.3.0.
Key Changes:
- Updated RoaringBitmap version from 1.3.0 to 1.6.0 in the version catalog
- Changed the Maven coordinates from
org.roaringbitmap:RoaringBitmaptocom.github.RoaringBitmap.RoaringBitmap:roaringbitmap - Updated relocation paths in all Spark runtime build files to reflect the new package structure
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Updated RoaringBitmap version to 1.6.0 and changed Maven coordinates to JitPack format |
| spark/v4.1/build.gradle | Added exclusion for old org.roaringbitmap group and updated relocation path for new package structure |
| spark/v4.0/build.gradle | Updated relocation path to match new RoaringBitmap package structure |
| spark/v3.5/build.gradle | Updated relocation path to match new RoaringBitmap package structure |
| spark/v3.4/build.gradle | Updated relocation path to match new RoaringBitmap package structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM the gradle usage is documented at https://github.com/RoaringBitmap/RoaringBitmap?tab=readme-ov-file#usage-within-a-gradle-project
Not sure if there's additional considerations when adding a new custom Maven repository, we should double check that
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
I think we'll need to double check any LICENSE contents that needs to be updated as well, looks like KC and the open API modules would need to be updated
|
@amogh-jahagirdar Updated LICENSE files. BTW, do you know why the LICENSE file in Filnk doesn't have version with it? |
39d7b65 to
f6d39d5
Compare
spark/v4.1/build.gradle
Outdated
| integrationImplementation ("org.apache.spark:spark-hive_${scalaVersion}:${libs.versions.spark41.get()}") { | ||
| exclude group: 'org.roaringbitmap' | ||
| } |
There was a problem hiding this comment.
Is this exclusion strictly required? Or are we just trying to slim down the spark-hive test dependencies?
There was a problem hiding this comment.
Otherwise, there will be two versions of Roaringbitmap on the test classpath.
There was a problem hiding this comment.
That makes sense, though is this not an issue also for the other spark versions? Did something get changed specifically within 4.1?
There was a problem hiding this comment.
Nice catch! I forgot to update for other Spark versions as well.
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Just had a question on why the exclusion is required, but other than that looks OK to me.
No, tbh I don't know why Flink doesn't track the explicit version. I don't think it's strictly required, at least as per https://infra.apache.org/licensing-howto.html#alv2-dep but yeah it is inconsistent with the other modules.
f6d39d5 to
ca29f72
Compare
ca29f72 to
ad01e6b
Compare
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @manuzhang! since it's a fairly critical dependency I'll give it another day for other people to chime in before merging.
|
@amogh-jahagirdar @kevinjqliu any more comments? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks @manuzhang and thanks @amogh-jahagirdar for the review |
Due to apache/iceberg#14991 and https://github.com/RoaringBitmap/RoaringBitmap not publishing to Maven Central, we need to declare in the jitpack.io repository.
The Iceberg PR apache/iceberg#14991 bumps the https://github.com/RoaringBitmap/RoaringBitmap version, which is not published to Maven Central. We need to declare in the jitpack.io repository to prepare for the next Iceberg release.
|
This PR affects the Iceberg downstream projects too. I see that other projects that uses roaringbitmap (like Apache Spark) done the same thing I see two issues: But If Iceberg has to support Spark 4.2 release, I understand that we need to add jitpack dependencies too. |
|
@ajantha-bhat Agreed. If we are going to use roaringbitmap in the long run, I don't see another way around it now. |
Due to apache/iceberg#14991 and https://github.com/RoaringBitmap/RoaringBitmap not publishing to Maven Central, we need to declare in the jitpack.io repository.
The Iceberg PR apache/iceberg#14991 bumps the https://github.com/RoaringBitmap/RoaringBitmap version, which is not published to Maven Central. We need to declare in the jitpack.io repository to prepare for the next Iceberg release.
The Iceberg PR apache/iceberg#14991 bumps the https://github.com/RoaringBitmap/RoaringBitmap version, which is not published to Maven Central. We need to declare in the jitpack.io repository to prepare for the next Iceberg release.
|
This is going to break anyone who pulls in unshaded iceberg artifacts until they add that new repo. What worries me is not less the change than the risk that jitpack isn't really a formal release process, more "jitpack finds and builds the right artifact when asked" Are these the official roaring bitmap artifacts, just served up to java build tools? |
I suppose. I followed the changes in Spark, and it looks jitpack repo has been used in a bunch of Apache projects besides Iceberg and Polaris. |
* Update actions/setup-java digest to be666c2 (apache#3527) * Update actions/setup-python digest to a309ff8 (apache#3528) * Update actions/stale digest to d6f8a33 (apache#3529) * Update dependency jupyterlab to v4.5.3 (apache#3530) * Update keycloak/keycloak Docker tag to v26.5.2 (apache#3536) * Update dependency com.fasterxml.jackson:jackson-bom to v2.21.0 (apache#3535) * Update quay.io/keycloak/keycloak Docker tag to v26.5.2 (apache#3533) * Update dependency org.assertj:assertj-core to v3.27.7 (apache#3531) * Update dependency com.diffplug.spotless:spotless-plugin-gradle to v8.2.0 (apache#3534) * fix(deps): update quarkus platform and group (apache#3532) * chore(deps): update postgres docker tag to v18 (apache#3541) * chore(deps): update localstack/localstack docker tag to v4 (apache#3540) * fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.26 (apache#3542) * fix(deps): update dependency software.amazon.awssdk:bom to v2.41.14 (apache#3546) * Add jitpack.io repository (apache#3504) The Iceberg PR apache/iceberg#14991 bumps the https://github.com/RoaringBitmap/RoaringBitmap version, which is not published to Maven Central. We need to declare in the jitpack.io repository to prepare for the next Iceberg release. * fix(deps): update dependency gradle.plugin.org.jetbrains.gradle.plugin.idea-ext:gradle-idea-ext to v1.4 (apache#3549) * Last merged commit 148bb74 --------- Co-authored-by: Mend Renovate <bot@renovateapp.com>
|
@manuzhang thanks. I will make sure the checksums are the same. I do note jitpack say "list us last", which means they'd prefer artifacts to come via other repos, where the authors can and should sign their artifacts, before going to jitpack. |
|
I went to see what binaries were in the roaring bitmap release, but they've only done source releases. https://github.com/RoaringBitmap/RoaringBitmap/releases/tag/1.6.0 commit is fec1dd5f05c I think it does need a mention in the release notes though: anyone pulling iceberg core through maven/gradle/sbt will get an error unless they work out where the artifact came from. |
|
Hey @steveloughran I agree, in fact I mean to start a dev list thread on this to make sure it doesn't fly under the radar. I think the upgrade is unavoidable but downstream consumers should be aware of the implications for sure. |
|
I'm actually less worried about this than anything else coming from jitpack when there are formal releases. For anyone with a corporate repo proxy/security scanner setup it also means involvement with them. |
roaringbitmap was no longer published to https://mvnrepository.com/artifact/org.roaringbitmap/RoaringBitmap after 1.3.0, but to https://jitpack.io as
com.github.RoaringBitmap.RoaringBitmap:roaringbitmap