[SPARK-31088][SQL] Add back HiveContext and createExternalTable #27815

gatorsmile · 2020-03-05T22:30:39Z

What changes were proposed in this pull request?

Based on the discussion in the mailing list [Proposal] Modification to Spark's Semantic Versioning Policy , this PR is to add back the following APIs whose maintenance cost are relatively small.

HiveContext
createExternalTable APIs

Why are the changes needed?

Avoid breaking the APIs that are commonly used.

Does this PR introduce any user-facing change?

Adding back the APIs that were removed in 3.0 branch does not introduce the user-facing changes, because Spark 3.0 has not been released.

How was this patch tested?

add a new test suite for createExternalTable APIs.

SparkQA · 2020-03-06T02:07:07Z

Test build #119423 has finished for PR 27815 at commit 294337d.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveContext(SQLContext):

dongjoon-hyun

Unfortunately, HiveContext is more than alias-level-maintenance. And, technically, this falls into a migration from 1.6.x age.
cc @marmbrus and @gatorsmile

dongjoon-hyun · 2020-03-09T01:15:42Z

Retest this please.

SparkQA · 2020-03-09T03:54:21Z

Test build #119546 has finished for PR 27815 at commit 294337d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveContext(SQLContext):

SparkQA · 2020-03-09T10:07:45Z

Test build #119555 has finished for PR 27815 at commit ff2d91f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class DeprecatedCreateExternalTableSuite extends SharedSparkSession

dongjoon-hyun · 2020-03-10T23:18:51Z

python/pyspark/sql/context.py

+    :param jhiveContext: An optional JVM Scala HiveContext. If set, we do not instantiate a new
+        :class:`HiveContext` in the JVM, instead we make all calls to this object.
+
+    .. note:: Deprecated in 2.0.0. Use SparkSession.builder.enableHiveSupport().getOrCreate().


Please rethink about this. Although the other removed API are recovered, I believe we should not recover this.
cc @marmbrus

Can you justify this position using the rubric we agreed upon?

I believe @gatorsmile should justify this using the new rubric about why this is maintenance cost are relatively small. We are reviewing this PR.
Also, cc @rxin since he is the release manager for 3.0.0.

Look at the code in this PR. I see a bunch of alias methods that call other methods and a few minimal tests. What part of this is hard to maintain?

class HiveContext(SQLContext) itself is not a simple alias. You know that. This is a whole new layer from Spark 1.6.x age.

I see 63 lines of code in that file? That class itself looks like an alias... not a whole separate implementation. What am I missing?

It seems that we have different ideas on the alias. You are saying that all wrappers are alias, aren't you?

If you are just delegating to the "correct"/new implementation, then that sounds like an alias to me. If we are maintaining large parallel/duplication implementation, then that is a different story.

SparkQA · 2020-03-20T07:05:02Z

Test build #120078 has finished for PR 27815 at commit c3a45e0.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-03-21T03:53:25Z

Test build #4996 has finished for PR 27815 at commit c3a45e0.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

gatorsmile · 2020-03-24T06:24:35Z

CC @dongjoon-hyun @marmbrus @rxin @srowen This PR is ready for review.

SparkQA · 2020-03-24T07:05:01Z

Test build #120242 has finished for PR 27815 at commit 1f1a570.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
final class ANOVASelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
final class VarianceThresholdSelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
//case class Foo(i: Int) extends AnyValwill return typeIntinstead ofFoo.

srowen

Same comment as elsewhere: keeping aliases is easy, so it's hard to feel strongly about it either way. I see the value in not changing APIs even across major versions. So, why were these deprecated in the first place, if you intended to never remove them, and why has that only come up now? Going forward, I assume we are just not going to deprecate anything, as nothing will realistically be removed

SparkQA · 2020-03-24T18:44:02Z

Test build #4997 has finished for PR 27815 at commit 1f1a570.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
final class ANOVASelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
final class VarianceThresholdSelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
//case class Foo(i: Int) extends AnyValwill return typeIntinstead ofFoo.

dongjoon-hyun · 2020-03-24T19:10:02Z

I also still strongly believe that we should let this go away from 3.0.0.

gatorsmile · 2020-03-24T20:36:05Z

Adoption of Spark 3.0 is much more important than removal of these deprecated methods. I strongly disagree we removed this APIs in Spark 3.0 at the beginning.

gatorsmile · 2020-03-24T20:55:15Z

retest this please

gatorsmile · 2020-03-24T21:14:07Z

To eventually remove these deprecated APIs, we need at least build some migration tools for end users to collect the usage of the deprecated APIs. So far, this is missing based on my knowledge. In general, I believe we need a systematic way for reduce the migration pain and make more end users easily upgrade to the latest version of Apache Spark.

SparkQA · 2020-03-25T00:23:36Z

Test build #120288 has finished for PR 27815 at commit 1f1a570.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
final class ANOVASelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
final class VarianceThresholdSelector @Since(\"3.1.0\")(@Since(\"3.1.0\") override val uid: String)
//case class Foo(i: Int) extends AnyValwill return typeIntinstead ofFoo.

srowen · 2020-03-25T04:16:46Z

@gatorsmile where did you disagree with this removal originally?

gatorsmile · 2020-03-25T04:47:12Z

Let me correct the typo and rewrite my original reply to make my point clear.

Based on the latest discussion in the community, I strongly disagree that we removed these APIs in Spark 3.0 initially. I think we need to add them back. If we want to remove them, we need more public discussions in the community. This PR is just to add the removed APIs back and delay the related discussions to the future releases.

dongjoon-hyun · 2020-03-25T05:26:00Z

Hi, All.
Although I have been against this reverting PR until now, I'll not be against any longer if @gatorsmile has his reason to support this.

gatorsmile · 2020-03-25T05:36:44Z

There are three major reasons why I think we should keep HiveContext:

SQLContext will not be removed in the near future, because it is part of DSV1 APIs. HiveContext is just the corresponding parts with Hive metastore support.
SQLContext/HiveContext is being used as the function parameter in many internal/public libraries. Replace it by SparkSession is not straightforward. It requires massive updates when end users upgrade Spark from 2.x to 3.x.
SQLContext/HiveContext is being documented in various tutorials and books.

srowen · 2020-03-25T06:54:21Z

@gatorsmile these changes happened almost a year ago. I'd like to understand why you have advanced these arguments only now. Has something changed?

gatorsmile · 2020-03-25T07:01:51Z

@srowen Like the previous releases, we are doing the API auditing and checking all the changes we made in the upcoming 3.0 release. During the auditing, we found it and multiple Spark committers (including @marmbrus @zsxwing @yhuai ) challenged this API change in our offline discussion. Thus, I submitted this PR to add them back.

cloud-fan

LGTM. Since we keep SQLContext as well, this is consistent to also keep HiveContext.

### What changes were proposed in this pull request? Based on the discussion in the mailing list [[Proposal] Modification to Spark's Semantic Versioning Policy](http://apache-spark-developers-list.1001551.n3.nabble.com/Proposal-Modification-to-Spark-s-Semantic-Versioning-Policy-td28938.html) , this PR is to add back the following APIs whose maintenance cost are relatively small. - HiveContext - createExternalTable APIs ### Why are the changes needed? Avoid breaking the APIs that are commonly used. ### Does this PR introduce any user-facing change? Adding back the APIs that were removed in 3.0 branch does not introduce the user-facing changes, because Spark 3.0 has not been released. ### How was this patch tested? add a new test suite for createExternalTable APIs. Closes #27815 from gatorsmile/addAPIsBack. Lead-authored-by: gatorsmile <[email protected]> Co-authored-by: yi.wu <[email protected]> Signed-off-by: gatorsmile <[email protected]> (cherry picked from commit b9eafcb) Signed-off-by: gatorsmile <[email protected]>

### What changes were proposed in this pull request? Based on the discussion in the mailing list [[Proposal] Modification to Spark's Semantic Versioning Policy](http://apache-spark-developers-list.1001551.n3.nabble.com/Proposal-Modification-to-Spark-s-Semantic-Versioning-Policy-td28938.html) , this PR is to add back the following APIs whose maintenance cost are relatively small. - HiveContext - createExternalTable APIs ### Why are the changes needed? Avoid breaking the APIs that are commonly used. ### Does this PR introduce any user-facing change? Adding back the APIs that were removed in 3.0 branch does not introduce the user-facing changes, because Spark 3.0 has not been released. ### How was this patch tested? add a new test suite for createExternalTable APIs. Closes apache#27815 from gatorsmile/addAPIsBack. Lead-authored-by: gatorsmile <[email protected]> Co-authored-by: yi.wu <[email protected]> Signed-off-by: gatorsmile <[email protected]>

add back HiveContext and createExternalTable

294337d

dongjoon-hyun requested changes Mar 9, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-28980][FOLLOW-UP] Add back HiveContext and createExternalTable~~ [SPARK-31088][SQL] Add back HiveContext and createExternalTable Mar 9, 2020

add DeprecatedCreateExternalTableSuite

ff2d91f

dongjoon-hyun requested changes Mar 10, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into pr-27815

c3a45e0

gatorsmile marked this pull request as ready for review March 20, 2020 04:51

dongjoon-hyun added the SQL label Mar 20, 2020

Merge remote-tracking branch 'upstream/master' into pr-27815

1f1a570

srowen reviewed Mar 24, 2020

View reviewed changes

dongjoon-hyun approved these changes Mar 25, 2020

View reviewed changes

cloud-fan approved these changes Mar 27, 2020

View reviewed changes

gatorsmile closed this in b9eafcb Mar 27, 2020

zero323 mentioned this pull request May 18, 2020

[SPARK-31088] Add back HiveContext and createExternalTable zero323/pyspark-stubs#402

Closed

HyukjinKwon mentioned this pull request Aug 7, 2023

[SPARK-44685][SQL] Remove deprecated Catalog#createExternalTable #42356

Closed

[SPARK-31088][SQL] Add back HiveContext and createExternalTable #27815

[SPARK-31088][SQL] Add back HiveContext and createExternalTable #27815

Uh oh!

Conversation

gatorsmile commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

SparkQA commented Mar 6, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Mar 9, 2020

Uh oh!

SparkQA commented Mar 9, 2020

Uh oh!

SparkQA commented Mar 9, 2020

Uh oh!

dongjoon-hyun Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

marmbrus Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 11, 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!

marmbrus Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 11, 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!

marmbrus Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

marmbrus Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 20, 2020

Uh oh!

SparkQA commented Mar 21, 2020

Uh oh!

gatorsmile commented Mar 24, 2020

Uh oh!

SparkQA commented Mar 24, 2020

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 24, 2020

Uh oh!

dongjoon-hyun commented Mar 24, 2020

Uh oh!

gatorsmile commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gatorsmile commented Mar 24, 2020

Uh oh!

gatorsmile commented Mar 24, 2020

Uh oh!

SparkQA commented Mar 25, 2020

Uh oh!

srowen commented Mar 25, 2020

Uh oh!

gatorsmile commented Mar 25, 2020

Uh oh!

dongjoon-hyun commented Mar 25, 2020

Uh oh!

gatorsmile commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

gatorsmile commented Mar 5, 2020 •

edited

Loading

dongjoon-hyun Mar 11, 2020 •

edited

Loading

dongjoon-hyun Mar 11, 2020 •

edited

Loading

gatorsmile commented Mar 24, 2020 •

edited

Loading

gatorsmile commented Mar 25, 2020 •

edited

Loading