Skip to content

[SPARK-27349][SQL] Dealing with TimeVars removed in Hive 2.x #24277

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SPARK-27349][SQL] Dealing with TimeVars removed in Hive 2.x #24277

wants to merge 6 commits into from

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Apr 3, 2019

What changes were proposed in this pull request?

hive.stats.jdbc.timeout and hive.stats.retries.wait were removed by HIVE-12164.
This pr to deal with this change.

How was this patch tested?

unit tests

wangyum
wangyum commented Apr 3, 2019
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala

// The following configurations were removed by HIVE-12164(Hive 2.0)
val removedTimeVars = Seq(
("hive.stats.jdbc.timeout", "30s") -> TimeUnit.SECONDS,
Copy link
Member Author

@wangyum wangyum Apr 3, 2019

Choose a reason for hiding this comment

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

The default value from HiveConf:
https://github.com/apache/hive/blob/release-1.2.1/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java#L1287
https://github.com/apache/hive/blob/release-1.2.1/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java#L1294

@wangyum wangyum mentioned this pull request Apr 3, 2019
Closed
@SparkQA
Copy link

SparkQA commented Apr 3, 2019

Test build #104227 has finished for PR 24277 at commit e67de70.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 3, 2019

Test build #104226 has finished for PR 24277 at commit 0dc62bf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

attilapiros
attilapiros reviewed Apr 3, 2019
View reviewed changes
Copy link
Contributor

@attilapiros attilapiros left a comment

Choose a reason for hiding this comment

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

What about extending a test with two more asserts checking the default values in case of an empty configuration?

Otherwise LGTM.

(Thanks for referencing the default values from Hive.)

@SparkQA
Copy link

SparkQA commented Apr 3, 2019

Test build #104250 has finished for PR 24277 at commit 1b7d636.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

gatorsmile
gatorsmile reviewed Apr 3, 2019
View reviewed changes
Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

gatorsmile
gatorsmile reviewed Apr 3, 2019
View reviewed changes
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala Outdated
}

// The following configurations were removed by HIVE-12164(Hive 2.0)
val removedTimeVars = Seq(
Copy link
Member

@gatorsmile gatorsmile Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

-> hardcodingTimeVars

gatorsmile
gatorsmile reviewed Apr 3, 2019
View reviewed changes
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveUtilsSuite.scala Outdated
assert(HiveUtils.formatTimeVarsForHiveClient(defaultConf)("hive.stats.jdbc.timeout") === "30")
assert(HiveUtils.formatTimeVarsForHiveClient(defaultConf)("hive.stats.retries.wait") === "3000")

def testFormatTimeVarsForHiveClient(key: String, value: String, expected: Long): Unit = {
Copy link
Member

@gatorsmile gatorsmile Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Move this out of the test case.

@SparkQA
Copy link

SparkQA commented Apr 4, 2019

Test build #104262 has finished for PR 24277 at commit 50ae570.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Apr 4, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Apr 4, 2019

Test build #104261 has finished for PR 24277 at commit 2d44ffd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 4, 2019

Test build #104263 has finished for PR 24277 at commit 50ae570.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Apr 4, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Apr 4, 2019

Test build #104267 has finished for PR 24277 at commit 50ae570.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

gatorsmile commented Apr 4, 2019

Thanks! Merged to master.

@gatorsmile gatorsmile closed this in 1d95dea Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@attilapiros attilapiros attilapiros left review comments

@gatorsmile gatorsmile gatorsmile left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wangyum @SparkQA @gatorsmile @attilapiros