-
Notifications
You must be signed in to change notification settings - Fork 29k
[DO-NOT-MERGE] Try to update Hive to 2.3.2 #20659
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
Conversation
* Update Hive to 2.3.2
|
Test build #87611 has finished for PR 20659 at commit
|
|
Test build #87613 has finished for PR 20659 at commit
|
|
Test build #87616 has finished for PR 20659 at commit
|
# Conflicts: # sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala # sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveShim.scala # sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala # sql/hive/src/main/scala/org/apache/spark/sql/hive/client/package.scala # sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/SaveAsHiveFile.scala # sql/hive/src/test/scala/org/apache/spark/sql/hive/client/VersionsSuite.scala
|
Test build #87960 has finished for PR 20659 at commit
|
|
Nice try! Could you fix the remaining failure? |
|
Yes, I'm doing it |
| <dependency> | ||
| <groupId>org.apache.hive</groupId> | ||
| <artifactId>hive-storage-api</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, you should not introduce hive into sql/core.
Apache ORC should be used as Apache ORC.
| <dependency> | ||
| <groupId>org.apache.orc</groupId> | ||
| <artifactId>orc-core</artifactId> | ||
| <classifier>${orc.classifier}</classifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't remove this.
| import org.apache.hadoop.conf.Configuration; | ||
| import org.apache.hadoop.hive.common.type.HiveDecimal; | ||
| import org.apache.hadoop.hive.ql.exec.vector.*; | ||
| import org.apache.hadoop.hive.serde2.io.HiveDecimalWritable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
| withTable("jt2") { | ||
| withView("testView") { | ||
| sql("CREATE VIEW testView AS SELECT id FROM jt") | ||
| sql("CREATE VIEW testView AS SELECT 1 as c1 FROM jt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unrelated change and looks wrong.
| sql("ALTER TABLE t1 ADD COLUMNS (C2 string)") | ||
| assert(spark.table("t1").schema == | ||
| new StructType().add("c1", IntegerType).add("C1", StringType)) | ||
| new StructType().add("c1", IntegerType).add("C2", StringType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are intentionally removing case sensitive test.
|
In general, this should keep all existing test coverage. Otherwise, it will not considered as a feasible solution. |
|
just out of curiosity, what does DNM stands for? |
|
I guess |
|
This is a very interesting work. Thanks for your effort! When we implement such a feature, please remember we have to ensure it is configurable. |
|
@gatorsmile @cloud-fan what is our plan for such thing? Currently we're using our own forked version, which blocks the support of Hadoop 3 (SPARK-18673). I'm wondering if we're going to upgrade Hive like what is done in this PR, or we still uses forked branch, and patch that branch. What is your thought and plan on it? |
|
@wangyum can you please create a relative JIRA, so that we discuss on the JIRA. I think it is better to discuss and get a conclusion first. |
|
Test build #88206 has finished for PR 20659 at commit
|
|
Test build #88217 has finished for PR 20659 at commit
|
|
retest this please |
|
Test build #88260 has finished for PR 20659 at commit
|
|
Test build #88269 has finished for PR 20659 at commit
|
|
retest this please |
|
Test build #88275 has finished for PR 20659 at commit
|
|
Test build #88299 has finished for PR 20659 at commit
|
|
Test build #88310 has finished for PR 20659 at commit
|
|
Test build #88311 has finished for PR 20659 at commit
|
|
retest this please |
|
Test build #88317 has finished for PR 20659 at commit
|
|
Thanks everyone, we can move to SPARK-23710 to discuss. |
|
|
||
| package org.apache.spark.sql.execution.datasources.orc | ||
|
|
||
| import org.apache.hadoop.hive.serde2.io.{DateWritable, HiveDecimalWritable} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
| import org.apache.orc.storage.serde2.io.HiveDecimalWritable | ||
| import org.apache.hadoop.hive.ql.io.sarg.{PredicateLeaf, SearchArgument, SearchArgumentFactory} | ||
| import org.apache.hadoop.hive.ql.io.sarg.SearchArgument.Builder | ||
| import org.apache.hadoop.hive.serde2.io.HiveDecimalWritable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
| import org.apache.orc.TypeDescription | ||
| import org.apache.orc.mapred.{OrcList, OrcMap, OrcStruct, OrcTimestamp} | ||
| import org.apache.orc.storage.common.`type`.HiveDecimal | ||
| import org.apache.orc.storage.serde2.io.{DateWritable, HiveDecimalWritable} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
| import scala.collection.JavaConverters._ | ||
|
|
||
| import org.apache.orc.storage.ql.io.sarg.{PredicateLeaf, SearchArgument} | ||
| import org.apache.hadoop.hive.ql.io.sarg.{PredicateLeaf, SearchArgument} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
|
I added more specific comments about ORC parts, @wangyum . |
|
Test build #88337 has finished for PR 20659 at commit
|
|
retest this please |
|
Test build #88338 has finished for PR 20659 at commit
|
|
What will take to merge this PR in? It has passed all the tests. |
|
@wangyum you can close this experiment? |
What changes were proposed in this pull request?
Check if there is any test failed.