search

substrait-io / substrait-java

Apache License 2.0
72 stars 70 forks source link

Shade ANTLR to avoid version conflicts with other packages #259

Closed bestbeforetoday closed 4 weeks ago

bestbeforetoday commented 1 month ago

ANTLR appears to be particularly prone to breakages due to version differences. When substrait-core is used with other packages that also have dependencies on ANTLR (such as Spark), differences in ANTLR dependency versions frequently cause breakages.

For example, depending on which dependency version is picked up first:

ANTLR Tool version 4.9.3 used for code generation does not match the current runtime version 4.13.1
ANTLR Runtime version 4.9.3 used for parser compilation does not match the current runtime version 4.13.1

or:

ANTLR Tool version 4.13.1 used for code generation does not match the current runtime version 4.9.3
ANTLR Runtime version 4.13.1 used for parser compilation does not match the current runtime version 4.9.3

I propose solving this issue by shading the ANTLR dependency into substrait-core using the Gradle shadow plugin. An example of prior art for this approach can be found in this issue and its corresponding code change.

bestbeforetoday commented 1 month ago

If you agree with the approach, I would be happy to try to deliver a fix.

Blizzara commented 1 month ago

I at least totally agree with the issue - and already made a PR for the fix: https://github.com/substrait-io/substrait-java/pull/258 ;) Would just need to get it reviewed

bestbeforetoday commented 1 month ago

@Blizzara I gave your change a quick try locally but unfortunately it didn't build for me. First a formatting violation and then — when I fixed the formatting — 463 test failures due to NoClassDefFoundError.

Since I made a similar change yesterday, I've raised that in PR #260. Any additional improvements are very welcome!

Blizzara commented 1 month ago

Yeah I see, I fixed the formatting but Isthmus tests fail due to NoClassDefFoundError. Adding jar { finalizedBy(shadowJar) } would probably help but then gradle complains because the dependencies are not handled correctly...

Blizzara commented 1 month ago

I pushed a version of my PR which should now work, though I find the way I got it to run a bit questionable. Happy to defer to yours if you find something better!

bestbeforetoday commented 4 weeks ago

Released in v0.32.0.