HIVE-24551: Hive should include transitive dependencies from calcite after shading it #1792

sunchao · 2020-12-17T02:31:21Z

What changes were proposed in this pull request?

This includes calcite-core's transitive dependencies:

net.hydromatic:eigenbase-properties
org.codehaus.janino:janino
org.codehaus.janino:commons-compiler
org.pentaho:pentaho-aggdesigner-algorithm

in the fat hive-exec jar.

Why are the changes needed?

Currently as part of effort of shading Guava from Hive, we shade Calcite and exclude all its artifacts from the binary distribution. However, this also removes all its transitive dependencies which will still needed at runtime. Without these, Hive queries will fail with error like:

Exception in thread "main" java.lang.NoClassDefFoundError: org/eigenbase/util/property/BooleanProperty
        at org.apache.calcite.util.SaffronProperties.<init>(SaffronProperties.java:66)
        at org.apache.calcite.util.SaffronProperties.instance(SaffronProperties.java:134)
        at org.apache.calcite.util.Util.getDefaultCharset(Util.java:769)
        at org.apache.calcite.rel.type.RelDataTypeFactoryImpl.getDefaultCharset(RelDataTypeFactoryImpl.java:565)
        at org.apache.calcite.sql.type.SqlTypeUtil.addCharsetAndCollation(SqlTypeUtil.java:1070)
        at org.apache.calcite.sql.type.SqlTypeFactoryImpl.createSqlType(SqlTypeFactoryImpl.java:65)
        at org.apache.calcite.rex.RexBuilder.<init>(RexBuilder.java:114)
        at org.apache.calcite.prepare.CalcitePrepareImpl.perform(CalcitePrepareImpl.java:991)
        at org.apache.calcite.tools.Frameworks.withPrepare(Frameworks.java:149)
        at org.apache.calcite.tools.Frameworks.withPlanner(Frameworks.java:106)
        at org.apache.hadoop.hive.ql.parse.CalcitePlanner.logicalPlan(CalcitePlanner.java:1069)
        at org.apache.hadoop.hive.ql.parse.CalcitePlanner.getOptimizedAST(CalcitePlanner.java:1085)
        at org.apache.hadoop.hive.ql.parse.CalcitePlanner.genOPTree(CalcitePlanner.java:364)
        at org.apache.hadoop.hive.ql.parse.SemanticAnalyzer.analyzeInternal(SemanticAnalyzer.java:11138)
        at org.apache.hadoop.hive.ql.parse.CalcitePlanner.analyzeInternal(CalcitePlanner.java:286)
        at org.apache.hadoop.hive.ql.parse.BaseSemanticAnalyzer.analyze(BaseSemanticAnalyzer.java:258)
        at org.apache.hadoop.hive.ql.Driver.compile(Driver.java:512)
        at org.apache.hadoop.hive.ql.Driver.compileInternal(Driver.java:1317)
        at org.apache.hadoop.hive.ql.Driver.runInternal(Driver.java:1457)
        at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1237)
        at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1227)
        at org.apache.hadoop.hive.cli.CliDriver.processLocalCmd(CliDriver.java:233)
        at org.apache.hadoop.hive.cli.CliDriver.processCmd(CliDriver.java:184)
        at org.apache.hadoop.hive.cli.CliDriver.processLine(CliDriver.java:403)
        at org.apache.hadoop.hive.cli.CliDriver.executeDriver(CliDriver.java:821)
        at org.apache.hadoop.hive.cli.CliDriver.run(CliDriver.java:759)
        at org.apache.hadoop.hive.cli.CliDriver.main(CliDriver.java:686)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at org.apache.hadoop.util.RunJar.run(RunJar.java:234)
        at org.apache.hadoop.util.RunJar.main(RunJar.java:148)
Caused by: java.lang.ClassNotFoundException: org.eigenbase.util.property.BooleanProperty
        at java.net.URLClassLoader.findClass(URLClassLoader.java:382)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
        at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
        ... 33 more

Does this PR introduce any user-facing change?

No

How was this patch tested?

Verified that the classes are included in the hive-exec jars with the change. I also manually tested this by launching a HS2 and run a simple query against it. It passed while previous was failing due to the above error.

sunchao · 2020-12-17T02:36:09Z

cc @viirya @dongjoon-hyun @szehon-ho

viirya · 2020-12-17T02:50:24Z

ql/pom.xml

+                  <!-- include all of calcite's transitive dependencies as well since
+                    otherwise they'll be removed in assembly packaging -->
+                  <include>net.hydromatic:eigenbase-properties</include>
+                  <include>org.codehaus.janino:janino</include>
+                  <include>org.codehaus.janino:commons-compiler</include>
+                  <include>org.pentaho:pentaho-aggdesigner-algorithm</include>


One question. Will this be flaky? Once calcite changes transitive dependencies, seems we also need to update this.

Yeah that's the downside of it - whenever we upgrade calcite we should check its dependencies and update here as well. I'm not sure if there is an easy way to include an artifact and all its transitive dependencies in the maven-shade-plugin

BTW, I remember in the latest branch, the shading guava PR doesn't include transitive dependencies like that. Do we also need to do it for the branch?

I still need to verify whether there is issue in master/4.0.0 as well - it uses calcite 1.21 which no longer depend on eigenbase that is causing the issue in our case, but I think it might be good to always include transitive dependencies anyway.

So I tried master branch and didn't hit the issue. However I think I may still open another PR for it since we don't really when the transitive dependencies will be needed at runtime.

Oh actually in master we already handles this here, as part of HIVE-22126. I think we should follow the same approach and explicitly add the transitive dependency in Hive's top-level pom. I think this is also how Spark manages its transitive dependencies? it might be better than pulling out the classes and putting them in the fat jar.

dongjoon-hyun · 2020-12-17T05:16:05Z

Thank you for pinging me, @sunchao .

sunchao · 2020-12-21T00:44:51Z

Updated the PR to fix a hanging issue when testing org.apache.hadoop.hive.cli.TestAccumuloCliDriver. Right now Hive is using Accumulo version 1.6.0 which depends on Thrift 0.9.0. However Hive uses Thrift 0.9.3 and because of this Accumulo TabletServer may fail with the following exception:

2020-12-20T12:02:18,577 ERROR [ClientPool 1[]] util.NamingThreadFactory: Thread "ClientPool 1" died tried to access field org.apache.thrift.server.AbstractNonblockingServer$FrameBuffer.trans_     from class org.apache.accumulo.server.util.TServerUtils$THsHaServer$Invocation
java.lang.IllegalAccessError: tried to access field org.apache.thrift.server.AbstractNonblockingServer$FrameBuffer.trans_ from class org.apache.accumulo.server.util.                               TServerUtils$THsHaServer$Invocation

which caused the test to hang. After upgrading Accumulo version to 1.7.3 (which is the same version used by Hive 3.0+) the issue is resolved.

HIVE-24551

1d1f542

kgyrtkirk added the tests pending label Dec 17, 2020

viirya approved these changes Dec 17, 2020

View reviewed changes

viirya reviewed Dec 17, 2020

View reviewed changes

dongjoon-hyun approved these changes Dec 17, 2020

View reviewed changes

szehon-ho approved these changes Dec 17, 2020

View reviewed changes

Empty commit to trigger CI again

700112a

kgyrtkirk added tests failed tests pending and removed tests pending tests failed labels Dec 18, 2020

sunchao added 2 commits December 17, 2020 18:42

revert previous change

2a97090

add dependencies instead

11f3d3b

kgyrtkirk added tests failed tests pending and removed tests pending tests failed labels Dec 19, 2020

sunchao mentioned this pull request Dec 19, 2020

HIVE-24553. Exclude calcite from test-jar dependency of hive-exec #1794

Merged

bump accumulo version to 1.7.3 and decrease test timeout to 4hr

09c4e3e

kgyrtkirk added tests failed tests pending tests unstable and removed tests pending tests failed tests unstable labels Dec 20, 2020

kgyrtkirk added the tests unstable label Dec 20, 2020

sunchao merged commit 52a4ab8 into apache:branch-2.3 Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HIVE-24551: Hive should include transitive dependencies from calcite after shading it #1792

HIVE-24551: Hive should include transitive dependencies from calcite after shading it #1792

Uh oh!

sunchao commented Dec 17, 2020

Uh oh!

sunchao commented Dec 17, 2020

Uh oh!

viirya Dec 17, 2020

Uh oh!

sunchao Dec 17, 2020

Uh oh!

viirya Dec 17, 2020

Uh oh!

sunchao Dec 17, 2020 •

edited

Loading

Uh oh!

sunchao Dec 18, 2020

Uh oh!

sunchao Dec 18, 2020

Uh oh!

dongjoon-hyun commented Dec 17, 2020

Uh oh!

sunchao commented Dec 21, 2020

Uh oh!

Uh oh!

HIVE-24551: Hive should include transitive dependencies from calcite after shading it #1792

HIVE-24551: Hive should include transitive dependencies from calcite after shading it #1792

Uh oh!

Conversation

sunchao commented Dec 17, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

sunchao commented Dec 17, 2020

Uh oh!

viirya Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

sunchao Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

viirya Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

sunchao Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunchao Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

sunchao Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 17, 2020

Uh oh!

sunchao commented Dec 21, 2020

Uh oh!

Uh oh!

sunchao Dec 17, 2020 •

edited

Loading